Repository navigation
Refs #5929 -- Added a CompositeField expression to support multi-column subqueries. - #21496
p-r-a-v-i-n wants to merge 37 commits into
Conversation
e8906a6 to
f3cd27d
Compare
f3cd27d to
cb6cd08
Compare
8ab4a57 to
eccb7c2
Compare
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Awesome @p-r-a-v-i-n, I'm looking forward to giving this a more substantive review tomorrow! Right now I just have one question for you about the tests.
11d8aa7 to
f5565ad
Compare
When the length of the subfield is 1
…in Query.output_field.
1015476 to
0e27eb4
Compare
0e27eb4 to
f14dab0
Compare
2312686 to
a28e36b
Compare
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Great stuff, I like how this is getting more and more concise! Left another round of comments.
| Using multi-column subqueries | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| A ``Subquery`` can also return multiple columns by calling ``values()`` with |
There was a problem hiding this comment.
Use Sphinx :meth: target for values().
|
|
||
| A ``Subquery`` can also return multiple columns by calling ``values()`` with | ||
| multiple field names. In this case, the subquery has a ``CompositeField`` | ||
| output. |
There was a problem hiding this comment.
sphinx target here also: output -> :ref: output_field<output-field>
|
|
||
| Models | ||
| ~~~~~~ | ||
| * Added support for CompositeField. CompositeField allow subqueries to |
There was a problem hiding this comment.
Good addition, but we still need to clarify the "Limiting a subquery to a single column" part, because it sounds like Subqueries always have to be limited, and this isn't clarified until later.
Something like:
... as the target of an __in lookup that does not expect a CompositeField, explained below (link).
🤔
| Using multi-column subqueries | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| A ``Subquery`` can also return multiple columns by calling ``values()`` with |
There was a problem hiding this comment.
Need versionadded:: 6.2 at the top of this section
|
|
||
| Models | ||
| ~~~~~~ | ||
| * Added support for CompositeField. CompositeField allow subqueries to |
There was a problem hiding this comment.
Blank line before the first bullet.
| if isinstance(self.lhs, ColPairs): | ||
| return repr(self.lhs.field.name) | ||
| elif hasattr(self.lhs, "output_field") and self.lhs.output_field.is_composite: | ||
| names = ", ".join(repr(name) for name in self.lhs.output_field.sub_fields) |
There was a problem hiding this comment.
Django style is to use list comprehensions instead of generators in cases like this (more verbose but micro-optimization)
| f"Cannot select the '{f}' alias. Use annotate() " | ||
| f"to promote it." | ||
| ) | ||
| elif f.split(LOOKUP_SEP, 1)[0] in self.annotations: |
There was a problem hiding this comment.
Annotations can have __ in them, so I don't think this logic will work (same as above). Let's add a test for an annotation with __ that exercises this. You can look into how this is handled generally.
There was a problem hiding this comment.
Yes , it is failing. i found in one place there also the similar logic was used and it was removed in the upstream.
There was a problem hiding this comment.
should we stop them from using such alias(__a__b__c__d______e__some=subquery) illegal alias ?
@jacobtylerwalls @LilyFirefly @charettes
Lily , Simon sorry for tagging here.
There was a problem hiding this comment.
Ah, I thought we could just piggyback on the support for double-underscores in annotations that we already have, but I forgot you can't use an annotation name to reference related object lookups anyway. For example, after .annotate(foo=F("some_fk")):
- you can do
foo__isnull - but you cannot do
foo__fk_field, you have to dosome_fk__fk_field
That makes sense, because the expression might be more complicated than just F.
So we really only support __ in annotation aliases when they're terminal. For this PR's new lookup functionality, we should throw a nice Python exception when it's attempted (either adding the annotation or doing the lookup--not sure), rather than possibly parse __ and chop at the wrong location.
| elif self.values_select and self.select: | ||
| is_composite = True |
There was a problem hiding this comment.
If len(self.values_select) == len(self.select) == 1, we don't have a composite field, but we end up going through CompositeField.from_select() anyway. That doesn't seem quite right.
e.g. in test_union_in_subquery():
(Pdb) self.select
(Col(queries_number, queries.Number.num),)
(Pdb) self.values_select
('num',)
There was a problem hiding this comment.
yeah that seems weird, we should not have composite columns when only a single row is selected.
| def get_lhs_str(self): | ||
| if isinstance(self.lhs, ColPairs): | ||
| return repr(self.lhs.field.name) | ||
| elif hasattr(self.lhs, "output_field") and self.lhs.output_field.is_composite: |
There was a problem hiding this comment.
Remind me why we need the defensive hasattr here and in process_lhs?
| if self.output_field.is_composite and self.output_field.has_one_field: | ||
| return resolved |
There was a problem hiding this comment.
This whole outer if branch avoids returning resolved, preferring either resolved.query or ExpressionWrapper(resolved.query, output_field=self.output_field), so at a glance I would expect this new branch to return one of thos two. Can you look at the blame to see what kind of test case we need to support here, and then add that test case?
LilyFirefly
left a comment
There was a problem hiding this comment.
I'm very excited to see this progress! I have a bunch of minor feedback (nitpicks and suggested refactors) and a few deeper questions.
| if annotation_select: | ||
| select_map = {} | ||
| for key, value in annotation_select.items(): | ||
| select_map[key] = extract_field(value) | ||
|
|
||
| if values_select: | ||
| select_map = {} | ||
| for path, sel in zip(values_select, select): | ||
| field = extract_field(sel) | ||
| select_map[path] = field |
There was a problem hiding this comment.
This looks odd! If both annotation_select and values_select are truthy, then select_map will only gain entries from values_select. I would expect to see either an exception raised if passing both is invalid, or for select_map to be built using a combination of annotation_select and values_select. If the current logic is intended, I think it needs a comment to explain why.
| select_map = {} | ||
| for key, value in annotation_select.items(): | ||
| select_map[key] = extract_field(value) |
There was a problem hiding this comment.
Nitpick, this could use a dictionary comprehension:
| select_map = {} | |
| for key, value in annotation_select.items(): | |
| select_map[key] = extract_field(value) | |
| select_map = {key: extract_field(value) for key, value in annotation_select.items()} |
| select_map[path] = field | ||
| elif annotation_select is None: | ||
| fields = [extract_field(sel) for sel in select] | ||
| select_map = {field.name: field for field in fields} |
There was a problem hiding this comment.
I think we need to check for field being None here:
| select_map = {field.name: field for field in fields} | |
| select_map = {field.name: field for field in fields if field} |
Possibly raising an exception would be better (it currently does, but we can likely do better than an AttributeError).
| def _make_composite(cls, name, value): | ||
| if isinstance(value, dict): | ||
| sub_fields = {k: cls._make_composite(k, v) for k, v in value.items()} | ||
| return cls(**sub_fields) | ||
| return value |
There was a problem hiding this comment.
This doesn't do anything with the name parameter. If that isn't needed by future (unwritten) code, we can simplify:
| def _make_composite(cls, name, value): | |
| if isinstance(value, dict): | |
| sub_fields = {k: cls._make_composite(k, v) for k, v in value.items()} | |
| return cls(**sub_fields) | |
| return value | |
| def _make_composite(cls, value): | |
| if isinstance(value, dict): | |
| sub_fields = {k: cls._make_composite(v) for k, v in value.items()} | |
| return cls(**sub_fields) | |
| return value |
| sub_fields = {k: cls._make_composite(k, v) for k, v in nested.items()} | ||
| return cls(**sub_fields) |
There was a problem hiding this comment.
I think this can use _make_composite more directly (with the removal of the name parameter I suggest below):
| sub_fields = {k: cls._make_composite(k, v) for k, v in nested.items()} | |
| return cls(**sub_fields) | |
| return cls._make_composite(nested) |
| def output_field(self): | ||
| if len(self.select) == 1: | ||
| select = self.select[0] |
There was a problem hiding this comment.
We seem to have lost this logic - was that intentional? I'd expect to see it in the if not is_composite: block below.
| if item in self.annotations: | ||
| if ( | ||
| item in self.annotations | ||
| or item.split(LOOKUP_SEP)[0] in self.annotations |
There was a problem hiding this comment.
Should this use the two-argument form of split, like below?
| or item.split(LOOKUP_SEP)[0] in self.annotations | |
| or item.split(LOOKUP_SEP, 1)[0] in self.annotations |
| condition = hasattr(self.output_field, "conditional") and isinstance( | ||
| self.output_field.conditional, fields.BooleanField | ||
| ) | ||
| return condition or isinstance(self.output_field, fields.BooleanField) |
There was a problem hiding this comment.
I think this would be equivalent, but maybe a bit simpler:
| condition = hasattr(self.output_field, "conditional") and isinstance( | |
| self.output_field.conditional, fields.BooleanField | |
| ) | |
| return condition or isinstance(self.output_field, fields.BooleanField) | |
| return isinstance(self.output_field, fields.BooleanField) or isinstance( | |
| getattr(self.output_field, "conditional", None), fields.BooleanField | |
| ) |
Or more verbosely:
| condition = hasattr(self.output_field, "conditional") and isinstance( | |
| self.output_field.conditional, fields.BooleanField | |
| ) | |
| return condition or isinstance(self.output_field, fields.BooleanField) | |
| if isinstance(self.output_field, fields.BooleanField): | |
| return True | |
| conditional = getattr(self.output_field, "conditional", None) | |
| if conditional is None: | |
| return False | |
| return isinstance(conditional, field.BooleanField) |
| return len(self.sub_fields) | ||
|
|
||
| @property | ||
| def has_one_field(self): |
There was a problem hiding this comment.
I see a lot of special-casing based on this - I wonder if it would be cleaner to require at least two fields in a CompositeField - I'm hoping we could special-case in a lot fewer places that way.
There was a problem hiding this comment.
That was my immediate reaction as well. I don't understand what the benefits of making one member composition of queryset aliases nested.
Isn't it easier to continue to make
Foo.objects.alias(
baz=Bar.objects.filter(pk=OuterRef("bar_id")).values("baz")
).filter(
baz="value"
)instead of also allowing filter(baz__baz="value") and potentially changing the plan of the generated query to perform a JOIN?
There was a problem hiding this comment.
it would be cleaner to require at least two fields in a CompositeField - I'm hoping we could special-case in a lot fewer places that way.
That sound reasonable.
| inner = CompositeField(email=EmailField(), age=IntegerField()) | ||
| outer = CompositeField(user=inner, title=CharField()) |
There was a problem hiding this comment.
Do we have a use-case for nesting CompositeField? I worry that it will generate a lot of edge-cases we need to handle carefully. If there's no use-case it might be cleaner to forbid it.
There was a problem hiding this comment.
@jacobtylerwalls , @charettes @LilyFirefly What's your thought should we allow ? and i agree with your concern. It's better to forbit it at least for now.
There was a problem hiding this comment.
Yeah, anything to reduce scope right now is a win.
charettes
left a comment
There was a problem hiding this comment.
Thanks for the work so far @p-r-a-v-i-n, I left a few comments.
| return [self] | ||
|
|
||
|
|
||
| class CompositeCol(Expression): |
There was a problem hiding this comment.
Do we really need this expression or could we do away with using Tuple instead?
I'm fairly confident a ton of complexity can be pruned by making tuple_lookups.Tuple.output_field a CompositeField with the output field of all of its source expressions.
There was a problem hiding this comment.
Sorry for taking a bit more time on this! I'm still exploring the Tuple idea, so please forgive me if I missed something, and Please correct my understanding
lets say we have sub = Post.objects.filter(user=OuterRef("pk")).values("title", "body")
and if we do User.objects.alias(posts=sub).values("posts__title") in such this case the outer table don't know about the title column. The outer table will receive a subquery . If i understand correctly .alias(info=subquery), the subquery's columns aren't addressable as Col(alias, field) from the outer query they only exist inside the subquery's scope.
and for info__user__email, to resolve user__email Tuple would need some mechanism to do "info"."user__email" from outer query.
And Tuple worked in case of CompositePrimaryKey bcs there wasn't any scenario where it has to look into subquery. and all column referencing to real models ?
There was a problem hiding this comment.
I'm interested in Simon's reply, but I have a feeling we should push harder on the Tuple idea and leave OuterRef support for later.
There was a problem hiding this comment.
in such this case the outer table don't know about the title column. The outer table will receive a subquery
From my understanding the outer query has a "posts" entry in its annotations map which points at the resolved sql.Query from sub that has an output_field of
output_field = CompositeField(
title=Col("posts", "title", Post._meta.get_field("title")),
body=Col("posts", "body", Post._meta.get_field("body")),
)in other words the alias of the Col of its output_field are the alias given to the FROM entry not the one local to the subquery. This binding of outer query alias to the annotation's output_field will likely need to happen at the same time as annotation is added as a member of alias_map.
So to summarize the sequence.
When doing
posts = Post.objects.filter(user=OuterRef("pk")).values("title", "body")
users = User.objects.alias(posts=posts)The users.query.annotations map is augmented with posts.resolve_expression -> sql.Query under "posts".
When posts is first referenced through filter or values (it's not part of alias_map) it gets added to alias_map and through the bounding process the annotation (or the table expression wrapper around it) gets cloned/relabelled so its output field now is aliased to "posts".
| target_field = output_field.related_model._meta.get_field(name) | ||
| return functools.partial( | ||
| CompositeCol, lookup_name=name, output_field=target_field | ||
| ) |
There was a problem hiding this comment.
I think this is too lax as nothing guarantees that the nested subquery has the appropriate JOIN already in place?
e.g. nothing guarantees that the subquery already has JOINed against user when it's aliased?
There was a problem hiding this comment.
The subquery is already baked with joins before it touch by CompositeCol. We here just mapping info__user__email to the correct colum of subquery's SELECT list.
for example this example in the case test_composite_nested_lookup_five_levels.
subquery = BugReport.objects.filter(
pk=self.bug_report.pk,
).values(
"task__name",
"task__project__title",
"task__project__workspace__name",
"task__project__workspace__owner__email",
"task__project__workspace__owner__first_name",
)
qs = User.objects.alias(ticket=subquery).values(
"ticket__task__project__workspace__owner__email",
"ticket__task__project__workspace__owner__first_name",
)
| elif self.values_select and self.select: | ||
| is_composite = True |
There was a problem hiding this comment.
yeah that seems weird, we should not have composite columns when only a single row is selected.
| lhs_is_raster = lookup.lhs.field.geom_type == "RASTER" | ||
| field = lookup.lhs.output_field | ||
| if field and field.is_composite and field.has_one_field: | ||
| field = field.output_field_when_only_one_subfield |
There was a problem hiding this comment.
Can't say I'm a big fan of these two properties (has_one_field and output_field_when_only_one_subfield).
Why can't we make singleton composite fields behave like non-composite ones?
There was a problem hiding this comment.
I agree with you. Honestly my intention was to make it more simple and clean but opposite happened.
| def get_lhs_len(self): | ||
| if hasattr(self.lhs, "output_field") and self.lhs.output_field.is_composite: | ||
| return len(self.lhs.output_field) | ||
| return len(self.lhs) |
There was a problem hiding this comment.
If we made the lhs a Tuple(Col(...), ...) instead of that CompositeCol I think we'd get that for free.
| CompositeCol( | ||
| CompositeCol(query, "user", ...), |
There was a problem hiding this comment.
It seems a bit off that CompositeCol references a sql.Query instance, this will cause problem on cloning (e.g. relabeling) as there is no memoization trough all query objects. In other words the query referenced by the expression and others won't point at the same object on cloning.
There was a problem hiding this comment.
May be any failing test case would expose this ? but i'm not able to come up with .
my hypothesis is here.
the root_lhs is always clone before use so it always get fresh clone before .as_sql . so the picture my mind is that bump_prefix is called and all the relabeling done before executing .as_sql.
| parts, root_lhs = self._resolve_lookup_path() | ||
| full_lookup = LOOKUP_SEP.join(parts) | ||
| query = root_lhs.clone() | ||
| query.set_values([full_lookup]) |
There was a problem hiding this comment.
Altering user provided queries could break its semantic.
Think the usage of DISTINCT for example; DISTINCT user.email, title and DISTINCT user.email don't yield the same results.
|
|
||
| qs = ( | ||
| User.objects.filter(pk=self.user1.pk) | ||
| .annotate(post_data=subquery) |
There was a problem hiding this comment.
Are we planning allow composite expressions to be annotated and not only aliased?
There was a problem hiding this comment.
I thought so, do you have a concern about that?
There was a problem hiding this comment.
Well it would definitely be interesting to have but I guess it's a bit more work particularly on backends don't support native row/tuple constructs as adding composite primary key support demonstrated.
For example, if you do
books = Book.objects.annotate(
author_details=Author.objects.filter(
pk=OuterRef("author_id"),
).values("first_name", "last_name")
)I assume users would expect books[0].author_details to be
class AuthorDetails(TypedDict):
first_name: str
last_name: strbut that poses challenges from a SQL generation perspective as we need to support annotations that spans more that one SELECT fields.
In other words, on backends that natively support tuple constructs we can do
SELECT
...,
(author.first_name, author.last_name) author_details
FROM book
JOIN LATERAL (
SELECT first_name, last_name
FROM author
WHERE id = book.id
) authorand then CompositeField.from_db_value can unpack the tuple to AuthorDetails but on backends that don't support such construct the tuple must be expanded into multiple aliases
SELECT
...,
author.first_name author_details__first_name,
author.last_name author_details__last_name
FROM book
JOIN LATERAL (
SELECT first_name, last_name
FROM author
WHERE id = book.id
) authorand then sliced, combined together to make their way to from_db_value. Not sure if composite_fields_to_tuples has been adapted to be able to deal with anything but composite primary keys and if it has it certainly didn't to handle for values and values_list variants (e.g. named=True).
There was a problem hiding this comment.
Ah. If it reduces scope for other reasons, I might be interested to remove it, but if it just boils down to Oracle versions below 21c, I'm okay not writing any fallback SQL for it and just raising an error. This isn't integral functionality like being able to count primary keys?
Uncovered lines in changed filesNote: Missing lines are warnings only. Some database-specific lines may not be measured. More information |
| self.assertNotIn('"info"', sql) | ||
| self.assertNotIn("`info`", sql) | ||
|
|
||
| def test_composite_test_annotations(self): |
There was a problem hiding this comment.
This test passes without the bc7fd5d changes. I think we don't need bc7fd5d, see #21496 (comment).
There was a problem hiding this comment.
This test was redudant (intention was to check nothing break when normal alias used) I will chop this one but the tests test_composite_test_terrible_alias and test_composite_outer_order_by will fail.
There was a problem hiding this comment.
Right, I think both tests should be changed to assert that an exception is raised, see my comment.
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Hey, I just left a few cosmetic comments before I lose track of them.
The more important pending points are removing the new changes in bc7fd5d, and exploring the Tuple idea and other related ideas from Lily & Simon's reviews.
| def get_lookup(self, lookup_name): | ||
| return super().get_lookup(lookup_name) |
| expressions in the same way as model fields. | ||
|
|
||
| A multi-column ``Subquery`` can also be compared as a whole using tuple | ||
| lookups, for example ``__lt``, ``__in``, and ``__gt``. |
There was a problem hiding this comment.
Use the markup:
:lookup:`lt`, :lookup:`gt`
etc
| class AbstractUser(models.Model): | ||
| email = models.EmailField(unique=True) | ||
| first_name = models.CharField(max_length=100) | ||
| age = models.IntegerField(default=24) | ||
|
|
||
| class Meta: | ||
| abstract = True | ||
|
|
||
|
|
||
| class User(AbstractUser): | ||
| pass |
There was a problem hiding this comment.
Can we just reuse the django.contrib.auth.models.User? Many other tests do this.
| return connection.ops.get_geom_placeholder_sql( | ||
| self.lhs.output_field, rhs, compiler | ||
| ) | ||
| field = self.lhs.output_field | ||
| return connection.ops.get_geom_placeholder_sql(field, rhs, compiler) |
| if lookup.lhs.output_field.geography and not self.geography: | ||
| field = lookup.lhs.output_field | ||
| if field.geography and not self.geography: |
| .annotate(the_email=F("info")) | ||
| .values("the_email") | ||
| ) | ||
| self.assertEqual(len(list(qs)), 3) |
There was a problem hiding this comment.
| self.assertEqual(len(list(qs)), 3) | |
| self.assertEqual(len(qs), 3) |
| with self.assertRaisesMessage( | ||
| ValueError, "'gt' lookup of ('id', 'email') must be a tuple or a list" | ||
| ): | ||
| list(User.objects.alias(info=subquery).filter(info__gt=5)) |
There was a problem hiding this comment.
We don't have to evaluate the qs to get the error. Also, we try to single-line these messages when we can.
| with self.assertRaisesMessage( | |
| ValueError, "'gt' lookup of ('id', 'email') must be a tuple or a list" | |
| ): | |
| list(User.objects.alias(info=subquery).filter(info__gt=5)) | |
| msg = "'gt' lookup of ('id', 'email') must be a tuple or a list" | |
| with self.assertRaisesMessage(ValueError, msg): | |
| User.objects.alias(info=subquery).filter(info__gt=5) |
| list(User.objects.alias(info=subquery).filter(info__gt=5)) | ||
|
|
||
|
|
||
| class SingleColumnValuesSubqueryTests(TestCaseSetup): |
There was a problem hiding this comment.
Curious about the separate class here.
| lhs_field = self.lhs._output_field_or_none | ||
| rhs_field = self.rhs._output_field_or_none | ||
| combined_type = _resolve_combined_type( | ||
| self.connector, | ||
| type(self.lhs._output_field_or_none), | ||
| type(self.rhs._output_field_or_none), | ||
| type(lhs_field), | ||
| type(rhs_field), |
This reverts commit bc7fd5d.
|
closing in favour of #21636 |
Trac ticket number
ticket-5929
Branch description
This PR Introduces a CompositeField to support subqueries which return multi-cols.
Currently Django ORM does not have internal machinery to decide the output_field of subqueries which have multi-columns in their output.
CompositeFieldwill resolveoutput_fieldfor such subqueries.Notes about this PR:
References:
AI Assistance Disclosure (REQUIRED)
Checklist
mainbranch.