#16436 closed Bug (fixed)
`annotate()` + `select_related()` + `only()` = `ValueError: invalid literal for int() with base 10`
Reported by: | Tai Lee | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Release blocker | Keywords: | annotate select_related only defer ValueError empty list |
Cc: | slafs@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
With some combinations of models and arguments to annotate()
, select_related()
and only()
, I get ValueError: invalid literal for int() with base 10
.
I think that SQLCompiler.results_iter() is not taking the deferred fields into account when calculating the aggregate_start
offset.
To create a test, I started with a large number of models each with a large number of fields, taken from an app that we have in production, and pared it down by trial and error to as simple a form as I could and still reproduce the error.
As I did this (choosing models and fields to remove at random) the behaviour changed from raising a ValueError
, to yielding no items when consuming the queryset (but with .count()
still returning a positive number), to apparently working as expected.
This is probably related to (or the same issue) that was reported in the comments of #11890.
I had a crack at calculating the correct aggregate_start
by looking at the deferred_loading
attribute and the q.get_loaded_field_names()
method on the Query
object, but didn't have any luck.
I've put the test in it's own module, defer_annotate_select_related_only
, just because it seems like such an obscure edge-case and I didn't want the other models and tests in defer_regress
to have any influence.
Hopefully if someone can figure out how and why this is happening, we can integrate a more concise test into defer_regress
.
Attachments (1)
Change History (20)
comment:1 by , 13 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
comment:2 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 13 years ago
Patch needs improvement: | unset |
---|
by , 13 years ago
Attachment: | 16436-annotate-select_related-only-r16522.diff added |
---|
Failing test case and fix.
comment:5 by , 13 years ago
I also noticed that a TypeError
raised in resolve_aggregate()
is silenced (not sure where), which causes the queryset to yield no results, even though .count()
returns a positive number. But other exceptions, such as ValueError
are not silenced.
This is why the symptoms of this bug vary between raising a ValueError
or yielding no results. If the aggregate_start
was calculated in such a way that a datetime
value was passed to resolve_aggregate()
for an ordinal aggregate such as Count
, a TypeError
exception is silenced. If a non-numerical string value is passed to an ordinal aggregate, a ValueError
exception is raised.
This patch fixes both cases by correctly calculating aggregate_start
, but it might also be a good idea to track down where TypeError
is being swallowed and fix that, too.
comment:7 by , 12 years ago
I've updated this patch on a branch at GitHub to apply cleanly.
https://github.com/thirstydigital/django/tree/tickets/16436-annotate-select_related-only
I think this is RFC. Does anyone have feedback or issues that would prevent this from being committed?
comment:8 by , 12 years ago
Component: | ORM aggregation → Database layer (models, ORM) |
---|
comment:10 by , 11 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
For me it seems to be ready to checkin.
comment:11 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:12 by , 11 years ago
Resolution: | fixed |
---|---|
Severity: | Normal → Release blocker |
Status: | closed → new |
Unfortunately the test doesn't pass under Oracle:
http://ci.djangoproject.com/job/Django%20Oracle/112/database=oracle,python=python2.7/testReport/junit/defer_regress.tests/DeferAnnotateSelectRelatedTest/test_defer_annotate_select_related/
Since it's a direct consequence of the patch, I'm re-opening the ticket and marking as a blocker.
comment:13 by , 11 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
comment:14 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:17 by , 11 years ago
comment:18 by , 11 years ago
Cc: | added |
---|
Replying to Tim Graham <timograham@…>:
Oracle doesn't like grouping by TextField, so use CharFields instead in
models.
Actually this is true for all types of LOB columns.
I have some serious doubts about whether or not this should be marked as fixed. If I get this right, we are in a situation where on Oracle one can't do any annotations on a model that has a TextField or BinaryField.
We should probably document this here https://docs.djangoproject.com/en/dev/ref/databases/#textfield-limitations (and add a note also about BinaryField) or try to fix this, by maybe introducing some parameter on a Query class. I noticed that there is some serious work regarding refactoring aggregate support (#14030). Is this issue also applicable for that kind of refactoring?
comment:19 by , 11 years ago
My understanding is that this can't be fixed in Django, this is a limitation in Oracle itself.
If documentation is needed for this issue then separate ticket seems like the way to go.
I've updated the patch with a couple more tests and a fix. The fix calculates the
aggregate_start
offset from loaded fields, if any are deferred, instead ofself.query.select
which includes all fields on the model.