add support for ordering by expressions #94
Conversation
timgraham
force-pushed
the
fix-ordering
branch
2 times, most recently
from
a84bf11 to
1988672
Compare
| else: | ||
| # The expression must be added to extra_fields with an alias. | ||
| field_name = f"__order{next(idx)}" | ||
| fields[field_name] = order.expression |
There was a problem hiding this comment.
todo: understand why this changes from extra_fields to fields in the second commit and update comment two lines above.
There was a problem hiding this comment.
the reason is:
The refs are actually references to specific fields selected in a query. These references compile to a position number within the as_sql method. To address this, I decided to move the refs to the post-project fields. Additionally, some of these refs are calculated during the project stage (such as annotations), so the refs are processed after the project stage. But the other projected fields that aren't refs have all the info to make the full expression.
Btw, 'refs' also has all the info to be compiled well, like an annotation with name (something that is not __annotation but isn't a column), it has the reference but also the full expression with it. So if we want to delete the extra field we could, the downside will be a field will be calculated twice or more.
There was a problem hiding this comment.
Even with the explanation, it's still difficult to understand. Now in this method, extra_fields is only used for nulls first/last, but then outside this method, all ordering_fields are added to extra_fields when columns is None (I guess because they won't be added to $project in that case). edit: I tried to clarify this with a comment.
django_mongodb/compiler.py
Outdated
| idx = itertools.count(start=1) | ||
| for order in self.order_by_expressions or []: | ||
| if isinstance(order.expression, Ref): | ||
| # TODO change? |
There was a problem hiding this comment.
todo: understand this to know whether it makes a difference if we use the commented version or the uncommented version.
There was a problem hiding this comment.
There are no differences. is okey to just use refs, but maybe we broke an abstraction? but in all part of the code we did (if this is the case)
There was a problem hiding this comment.
🤔 mmh now I see the idea, and you are right, we could ignore the refs projection, we don't need them, because they are already managed in $project. So we can remove the refs from the extra fields
| @@ -51,28 +50,11 @@ def __init__(self, compiler): | |||
| self.lookup_pipeline = None | |||
| self.project_fields = None | |||
| self.aggregation_pipeline = compiler.aggregation_pipeline | |||
| self.extra_fields = None | |||
There was a problem hiding this comment.
I think now we can pass the compiler alone and the query will know how to fill in all the fields from the compiler.
There was a problem hiding this comment.
This is an idea for a later cleanup, right?
There was a problem hiding this comment.
yes, to avoid doing
query.something = value
query.anotherthing = other_value
...
django_mongodb/aggregates.py
Outdated
| @@ -47,7 +47,7 @@ def count(self, compiler, connection, resolve_inner_expression=False, **extra_co | |||
| source_expressions = node.get_source_expressions() | |||
| filter_ = deepcopy(self.filter) | |||
| filter_.add( | |||
| WhereNode([Exact(source_expressions[0], Value(None))], negated=True), | |||
| WhereNode([IsNull(source_expressions[0], True)], negated=True), | |||
There was a problem hiding this comment.
I saw your comment about this in the previous PR, but does IT belong in this one? Maybe it affects a Django test we haven't triaged yet?
There was a problem hiding this comment.
Yes, it doesn't affect any current test, but I don't want to forget this tiny change. This IsNull is needed when the source_expression[0] is a foreign field. maybe we should create another ticket in order to make it more traceable ?
There was a problem hiding this comment.
Ok, thanks. We could merge it now. Just put it in a separate PR with the explanation, please. I'll remove it here when I squash the final edits. edit: I created #98.
The compiler can now handle aggregated fields in ordering, and several crashes have been fixed.