Skip to content

fix collisions with projected columns and order by columns #108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 22, 2024
Merged

fix collisions with projected columns and order by columns #108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
892f429
add aggregation_regress test skips
WaVEV Aug 19, 2024
9dd5f08
fix collisions with projected columns and order by columns
WaVEV Aug 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test-python.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ jobs:
run: >
python3 django_repo/tests/runtests.py --settings mongodb_settings -v 2
aggregation
aggregation_regress
annotations
auth_tests.test_models.UserManagerTestCase
backends.base.test_base.DatabaseWrapperTests
Expand Down
27 changes: 12 additions & 15 deletions django_mongodb/compiler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,7 @@ def build_query(self, columns=None):
if columns is None:
extra_fields += ordering_fields
if extra_fields:
query.extra_fields = {
field_name: expr.as_mql(self, self.connection) for field_name, expr in extra_fields
}
query.extra_fields = self.get_project_fields(extra_fields, force_expression=True)
where = self.get_where()
try:
expr = where.as_mql(self, self.connection) if where else {}
Expand Down Expand Up @@ -431,16 +429,18 @@ def _get_aggregate_expressions(self, expr):
elif hasattr(expr, "get_source_expressions"):
stack.extend(expr.get_source_expressions())

def get_project_fields(self, columns=None, ordering=None):
def get_project_fields(self, columns=None, ordering=None, force_expression=False):
if not columns:
return {}
fields = defaultdict(dict)
for name, expr in columns or []:
for name, expr in columns + (ordering or ()):
collection = expr.alias if isinstance(expr, Col) else None
try:
fields[collection][name] = (
1
# For brevity/simplicity, project {"field_name": 1}
# instead of {"field_name": "$field_name"}.
if isinstance(expr, Col) and name == expr.target.column
if isinstance(expr, Col) and name == expr.target.column and not force_expression
else expr.as_mql(self, self.connection)
)
except EmptyResultSet:
Expand All @@ -451,9 +451,6 @@ def get_project_fields(self, columns=None, ordering=None):
# should appear in the top-level of the fields dict.
fields.update(fields.pop(None, {}))
fields.update(fields.pop(self.collection_name, {}))
# Add order_by() fields.
if fields and ordering:
fields.update({alias: expr.as_mql(self, self.connection) for alias, expr in ordering})
# Convert defaultdict to dict so it doesn't appear as
# "defaultdict(<CLASS 'dict'>, ..." in query logging.
return dict(fields)
Expand All @@ -466,28 +463,28 @@ def _get_ordering(self):
- A tuple of ('field_name': Expression, ...) for expressions that need
to be added to extra_fields.
"""
fields = {}
fields = []
sort_ordering = SON()
extra_fields = {}
extra_fields = []
idx = itertools.count(start=1)
for order in self.order_by_objs or []:
if isinstance(order.expression, Col):
field_name = order.expression.as_mql(self, self.connection).removeprefix("$")
fields[field_name] = order.expression
fields.append((order.expression.target.column, order.expression))
elif isinstance(order.expression, Ref):
field_name = order.expression.as_mql(self, self.connection).removeprefix("$")
else:
field_name = f"__order{next(idx)}"
fields[field_name] = order.expression
fields.append((field_name, order.expression))
# If the expression is ordered by NULLS FIRST or NULLS LAST,
# add a field for sorting that's 1 if null or 0 if not.
if order.nulls_first or order.nulls_last:
null_fieldname = f"__order{next(idx)}"
condition = When(IsNull(order.expression, True), then=Value(1))
extra_fields[null_fieldname] = Case(condition, default=Value(0))
extra_fields.append((null_fieldname, Case(condition, default=Value(0))))
sort_ordering[null_fieldname] = DESCENDING if order.nulls_first else ASCENDING
sort_ordering[field_name] = DESCENDING if order.descending else ASCENDING
return tuple(fields.items()), sort_ordering, tuple(extra_fields.items())
return tuple(fields), sort_ordering, tuple(extra_fields)

def get_where(self):
return self.where
Expand Down
22 changes: 22 additions & 0 deletions django_mongodb/features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,16 @@ class DatabaseFeatures(BaseDatabaseFeatures):
"aggregation.tests.AggregateTestCase.test_aggregation_default_passed_another_aggregate",
"aggregation.tests.AggregateTestCase.test_annotation_expressions",
"aggregation.tests.AggregateTestCase.test_reverse_fkey_annotate",
"aggregation_regress.tests.AggregationTests.test_annotation_disjunction",
"aggregation_regress.tests.AggregationTests.test_decimal_aggregate_annotation_filter",
# QuerySet.extra(select=...) should raise NotSupportedError instead of:
# 'RawSQL' object has no attribute 'as_mql'.
"aggregation_regress.tests.AggregationTests.test_annotate_with_extra",
"aggregation_regress.tests.AggregationTests.test_annotation",
"aggregation_regress.tests.AggregationTests.test_more_more3",
"aggregation_regress.tests.AggregationTests.test_more_more_more3",
# Incorrect JOIN with GenericRelation gives incorrect results.
"aggregation_regress.tests.AggregationTests.test_aggregation_with_generic_reverse_relation",
}
# $bitAnd, #bitOr, and $bitXor are new in MongoDB 6.3.
_django_test_expected_failures_bitwise = {
Expand Down Expand Up @@ -208,6 +218,7 @@ def django_test_expected_failures(self):
"aggregation.tests.AggregateTestCase.test_exists_extra_where_with_aggregate",
"annotations.tests.NonAggregateAnnotationTestCase.test_annotation_exists_aggregate_values_chaining",
"annotations.tests.NonAggregateAnnotationTestCase.test_annotation_exists_none_query",
"aggregation_regress.tests.AggregationTests.test_annotate_and_join",
"delete_regress.tests.DeleteTests.test_self_reference_with_through_m2m_at_second_level",
"expressions.tests.BasicExpressionsTests.test_annotation_with_deeply_nested_outerref",
"expressions.tests.BasicExpressionsTests.test_boolean_expression_combined",
Expand Down Expand Up @@ -297,6 +308,13 @@ def django_test_expected_failures(self):
"annotations.tests.NonAggregateAnnotationTestCase.test_annotation_and_alias_filter_in_subquery",
"annotations.tests.NonAggregateAnnotationTestCase.test_annotation_and_alias_filter_related_in_subquery",
"annotations.tests.NonAggregateAnnotationTestCase.test_empty_expression_annotation",
"aggregation_regress.tests.AggregationTests.test_aggregates_in_where_clause",
"aggregation_regress.tests.AggregationTests.test_aggregates_in_where_clause_pre_eval",
"aggregation_regress.tests.AggregationTests.test_f_expression_annotation",
"aggregation_regress.tests.AggregationTests.test_having_subquery_select",
"aggregation_regress.tests.AggregationTests.test_more_more4",
"aggregation_regress.tests.AggregationTests.test_more_more_more5",
"aggregation_regress.tests.AggregationTests.test_negated_aggregation",
"db_functions.comparison.test_coalesce.CoalesceTests.test_empty_queryset",
"expressions_case.tests.CaseExpressionTests.test_annotate_with_in_clause",
"expressions.tests.FTimeDeltaTests.test_date_subquery_subtraction",
Expand Down Expand Up @@ -351,6 +369,7 @@ def django_test_expected_failures(self):
"QuerySet.dates() is not supported on MongoDB.": {
"aggregation.tests.AggregateTestCase.test_dates_with_aggregation",
"annotations.tests.AliasTests.test_dates_alias",
"aggregation_regress.tests.AggregationTests.test_more_more_more2",
"dates.tests.DatesTests.test_dates_trunc_datetime_fields",
"dates.tests.DatesTests.test_related_model_traverse",
},
Expand All @@ -368,6 +387,9 @@ def django_test_expected_failures(self):
},
"QuerySet.distinct() is not supported.": {
"aggregation.tests.AggregateTestCase.test_sum_distinct_aggregate",
"aggregation_regress.tests.AggregationTests.test_annotate_distinct_aggregate",
"aggregation_regress.tests.AggregationTests.test_conditional_aggregate_on_complex_condition",
"aggregation_regress.tests.AggregationTests.test_distinct_conditional_aggregate",
"lookup.tests.LookupTests.test_lookup_collision_distinct",
"ordering.tests.OrderingTests.test_orders_nulls_first_on_filtered_subquery",
"queries.tests.ExcludeTest17600.test_exclude_plain_distinct",
Expand Down