Refs #373 -- Added tuple lookups. #18404
Conversation
36bd654 to
9b0fa13
Compare
50d5d15 to
1a7d522
Compare
3385c0b to
46bbb8e
Compare
2f41437 to
0f23f0f
Compare
|
I changed |
|
buildbot, test on oracle. |
a3d0596 to
8fc4eb2
Compare
|
buildbot, test on oracle. |
| def check_tuple_lookup(self): | ||
| assert isinstance(self.lhs, ColPairs) | ||
| self.check_rhs_is_tuple_or_list() | ||
| self.check_rhs_length_equals_lhs_length() | ||
|
|
||
| def check_rhs_is_tuple_or_list(self): | ||
| if not isinstance(self.rhs, (tuple, list)): | ||
| raise ValueError( | ||
| f"'{self.lookup_name}' lookup of '{self.lhs.field.name}' field " | ||
| "must be a tuple or a list" | ||
| ) | ||
|
|
||
| def check_rhs_length_equals_lhs_length(self): | ||
| if len(self.lhs) != len(self.rhs): | ||
| raise ValueError( | ||
| f"'{self.lookup_name}' lookup of '{self.lhs.field.name}' field " | ||
| f"must have {len(self.lhs)} elements" | ||
| ) | ||
|
|
||
| def check_rhs_is_collection_of_tuples_or_lists(self): | ||
| if not all(isinstance(vals, (tuple, list)) for vals in self.rhs): | ||
| raise ValueError( | ||
| f"'{self.lookup_name}' lookup of '{self.lhs.field.name}' field " | ||
| f"must be a collection of tuples or lists" | ||
| ) | ||
|
|
||
| def check_rhs_elements_length_equals_lhs_length(self): | ||
| if not all(len(self.lhs) == len(vals) for vals in self.rhs): | ||
| raise ValueError( | ||
| f"'{self.lookup_name}' lookup of '{self.lhs.field.name}' field " | ||
| f"must have {len(self.lhs)} elements each" | ||
| ) |
There was a problem hiding this comment.
Why we don't have any tests for these checks? Secondly, some of them are used only in a single lookup so I'm not sure why there are in the mixin 🤔, and, as far as I'm aware, some of them are unnecessary, e.g. check_rhs_is_tuple_or_list().
We also should not call functions (e.g. len()) in f-strings as described in the contributing guide.
| class ColPairs(Expression): | ||
| def __init__(self, alias, targets, sources, output_field): | ||
| super().__init__(output_field=output_field) | ||
| self.alias, self.targets, self.sources = alias, targets, sources |
There was a problem hiding this comment.
Please don't use this pattern, we removed it from some places in the past (e.g. 4e73d8c).
Check out #16805 (comment).
|
Thanks for the feedback @felixxm .
As you noticed, these errors are mostly unreachable during normal code execution, they are more like sanity checks. Based on your feedback, I added tests for what I could Refs #373. I also test them in the follow-up composite primary key PR more thoroughly Fixed #373.
Because I liked having all checks in one place... we can move it if you think this is a problem.
I don't think sanity checks are unnecessary.
Ok, adjusted it in #18485. |
We normally don't add unreachable code to Django 😶 |
| @unittest.skipIf( | ||
| connection.vendor == "mysql", | ||
| "MySQL doesn't support LIMIT & IN/ALL/ANY/SOME subquery", | ||
| ) |
There was a problem hiding this comment.
Instead of connection.vendor checks, please use DatabaseFeatures.django_test_skips instead.
There was a problem hiding this comment.
We actually have a feature switch for that folks
django/django/db/backends/mysql/features.py
Lines 10 to 11 in 8864125
Original PR #18404 by csirmazbendeguz Original: django/django#18404
Merged from original PR #18404 Original: django/django#18404
Original PR #18404 by csirmazbendeguz Original: django/django#18404
Merged from original PR #18404 Original: django/django#18404
Trac ticket number
ticket-373
Branch description
This is a prerequisite for composite primary keys (#18056).
related_lookups.MultiColSource->expressions.Cols.Colswill also be used by composite primary keys.Fixed esoteric
ForeignObject"related" lookups.At the moment, the expression
(a, b, c) < (x, y, z)compiles to:WHERE a < x AND b < y AND c < z.This is incorrect, e.g.
(1, 9) < (2, 1)would evaluate as false.This PR addresses this bug by compiling the expression to:
WHERE a < x OR (a = x AND (b < y OR (b = y AND c < z)))instead.If the backend supports tuple comparisons, it's compiled to:
WHERE (a, b, c) < (x, y, z).This fix is applied to
<=,>,>=as well.=will use tuple comparison if the backend supports it (WHERE (a, b, c) = (x, y, z)).INwill use tuple comparison if the backend supports it ((a, b, c) IN ((x1, y1, z1), (x2, y2, z2))IS NULLhas no significant changes, only refactoring.Checklist
mainbranch.