Refs #24267 -- Replaced hardcoded null eliding by flags and methods. #16817
Conversation
| expr.is_nullable(nullable_aliases) for expr in self.get_source_expressions() | ||
| ) | ||
|
|
||
| def exclude_nulls(self, nullable_aliases): |
There was a problem hiding this comment.
Maybe elide_null or elide_unknown would be a better name?
There was a problem hiding this comment.
Devil's advocate: I'm a native english speaker and I had to look up the word "elide" when I first encountered it with migrations. I wonder if we should avoid using rarely used terms 🤔 I've had a few people ask what it means when I teach them about data migrations.
It's probably less of an issue if this is a private API… but still relevant for maintainers 🤷♂️
Edit: Just did a quick survey at $job and native english speakers here had either never heard of it or have but have never actually used it.
There was a problem hiding this comment.
We have elidable already in the migrations framework, so although I hear what you're saying, it doesn't bother me here. EDIT: ah right, I see you mention its use in the migrations framework, too. I guess I'm saying we've already paid the cost of using it :-)
django/db/models/sql/query.py
Outdated
| if self.alias_map[join_list[-1]].join_type == LOUTER: | ||
| nullable_aliases = {alias} | ||
| else: | ||
| nullable_aliases = {} |
There was a problem hiding this comment.
We'll need to pass the full chain of nullable aliases in there when dealing with conditional expressions, not needed for simple lookups though.
| # A subquery can always return no rows and thus be nullable but the ORM | ||
| # currently has some expectations with regards to IN vs ANY lookups that | ||
| # must be revisited to allow this logic to work properly. | ||
| return False |
There was a problem hiding this comment.
If we wanted to enable this we'd have to change our usage of field IN (subquery) to field = ANY (subquery), see ticket-20024 which is related.
| if ( | ||
| current_negated | ||
| and (lookup_type != "isnull" or condition.rhs is False) | ||
| and condition.rhs is not None |
There was a problem hiding this comment.
The removal of this JSONField specialized case is no more thanks to the handling of can_use_none_as_rhs in Lookup.exclude_nulls.
| product = Product.objects.create(qty_target=42) | ||
| self.assertEqual( | ||
| Product.objects.get( | ||
| Exact(Coalesce("stock__qty_available", "qty_target"), 42) |
There was a problem hiding this comment.
The exact use case brought up by Anssi a few year ago https://gist.github.com/akaariai/a4c1acbfedac0f2cbf3e
A LEFT JOIN against stock must be used for the value to be None and qty_target be used.
| current_negated=True, | ||
| branch_negated=True, | ||
| can_reuse=can_reuse, |
There was a problem hiding this comment.
These kwargs were originally passed along but didn't carry any meaning since passing an expression to build_filter didn't make use of them until now.
Not doing so causes queries.tests.ManyToManyExcludeTest failures with the optimization enabled.
| @@ -312,6 +312,22 @@ def _output_field_or_none(self): | |||
| if not self._output_field_resolved_to_none: | |||
| raise | |||
|
|
|||
| @cached_property | |||
| def constrains_nulls(self): | |||
There was a problem hiding this comment.
This property means does this expression requires NULLs to be provided to behave correctly. Only IsNull(..., True) and Coalesce have special null handling logic today but this flag allows other expressions to opt in instead of using hardcoded reference to lookup names.
|
@charettes @jacobtylerwalls We have had another (dupe) report about this issue (ticket-34959). @charettes would you have time to update/rebase this branch and fix conflicts? Thanks! |
28bac82 to
144bf2a
Compare
0827a61 to
2b463a5
Compare
No description provided.