INTPYTHON-833 Remove $facet in top level group stages #449

WaVEV · 2025-11-20T04:29:48Z

Generated query:

[{'$group': {'_id': None,
             'count': {'$sum': {'$cond': {'else': 1,
                                          'if': {'$in': [{'$type': '$_id'},
                                                         ['missing', 'null']]},
                                          'then': None}}}}},
 {'$addFields': {'__now': '$$NOW'}},
 {'$unionWith': {'pipeline': [{'$documents': [{}]}]}},
 {'$project': {'count': {'$ifNull': ['$count', {'$literal': 0}]}}}]```

aclark4life · 2025-11-20T14:05:27Z

Awesome, thank you and thanks @asya999

aclark4life

Can you add a test, particular one that would test something that breaks in the Django admin with QE (even though QE is not merged yet)?

timgraham · 2025-11-20T14:43:24Z

django_mongodb_backend/compiler.py

-                }
-            )
+            pipeline.append({"$group": group})
+            # It may be a bug, $$NOW has to be called to be reachable in the rest of the pipeline.


Please add link to the SERVER ticket and add an explanatory comment about the purpose of the $unionWith clause.

I think we can also mention it in the release notes as a performance improvement since the team said $facet prevents the use of optimizations.

Sadly, this cannot be implemented as it is.
To handle the global aggregation and the default values (for example:

SELECT coalesce(avg(age), 35) FROM people WHERE age > 100;

we need to wrap the aggregation so MongoDB always returns a single document.
This is done by wrapping the pipeline inside a lookup. Example:

db.z.aggregate([ { $collStats: {} }, { $lookup: { from: "z", as: "wrap", pipeline: [...pipeline stages] } }, { $replaceWith: {"$cond": [{"$eq": ["$wrap", []]}, {}, {$first: "$wrap"}]}} ])

It’s not very intuitive use of $collStats, but it works 😄. It replaces the need for $unionWith.
$collStats is used as a pivot so we can apply the required operator after the group result.

Well played (I think?) 😄

😆 The idea comes from the team

Jibola · 2025-11-21T18:48:26Z

Can you provide, in this PR, an example of what the removal of $facet makes the new query signatures look like?

timgraham · 2025-11-25T21:48:42Z

I updated the test on the Django fork so it won't fail.

timgraham

Need a test with assertAggregateQuery, perhaps in tests/aggregation_.

timgraham · 2025-12-02T01:04:54Z

django_mongodb_backend/aggregates.py

    # If distinct=True or resolve_inner_expression=False, sum the size of the
    # set.
    lhs_mql = process_lhs(self, compiler, connection, as_expr=True)
+    lhs_mql = {"$ifNull": [lhs_mql, []]}


Add a comment about what fails without this?

django_mongodb_backend/compiler.py

django_mongodb_backend/query.py

timgraham · 2025-12-03T23:45:42Z

django_mongodb_backend/query.py

+        if self.wrap_for_global_aggregation:
+            # Use $collStats as a pivot to guarantee a single input document
+            pipeline = [
+                {"$collStats": {}},


As discussed on Zoom, it sounds like $collStats causes extra filesystem access that could be avoided by using $document. If it's straightforward, use that for now and I'll add a condition to use $collStats only for encrypted connections in that PR.

WaVEV · 2025-12-05T03:00:13Z

django_mongodb_backend/query.py

+        if self.wrap_for_global_aggregation:
+            # Add an empty extra document to handle default values on empty results
+            pipeline.append({"$unionWith": {"pipeline": [{"$documents": [{}]}]}})
+        if self.having:
+            pipeline.append({"$match": self.having})


this make the test pass, but I think this should be tested in deep, need to add a test that having filter all the groups.

the current django tests cover the case and it is right.

timgraham · 2025-12-09T01:41:26Z

There is still some problem on evergreen / MongoDB 8:

ERROR: test_aggregation_default_using_duration_from_database (aggregation.tests.AggregateTestCase)
...
[2025/12/05 01:14:54.701]   File "/data/mci/6fa58471acf0c3d789a3318d07c79185/src/venv/lib/python3.10/site-packages/pymongo/helpers_shared.py", line 284, in _check_command_response
[2025/12/05 01:14:54.701]     raise OperationFailure(errmsg, code, response, max_wire_version)
[2025/12/05 01:14:54.701] pymongo.errors.OperationFailure: PlanExecutor error during aggregation :: caused by :: Builtin variable '$$NOW' is not available, full error: {'ok': 0.0, 'errmsg': "PlanExecutor error during aggregation :: caused by :: Builtin variable '$$NOW' is not available", 'code': 51144, 'codeName': 'Location51144'}

Do we need that workaround you had before?

timgraham · 2025-12-09T02:15:03Z

tests/model_fields_/test_arrayfield.py

+    def test_overlap_empty_values(self):
+        qs = NullableIntegerArrayModel.objects.filter(order__lt=-30)
+        self.assertCountEqual(
+            NullableIntegerArrayModel.objects.filter(
+                field__overlap=qs.values_list("field"),
+            ),
+            [],
+        )
+        self.assertCountEqual(
+            NullableIntegerArrayModel.objects.filter(
+                field__overlap=qs.values("field"),
+            ),
+            [],
+        )


I believe this is additional test coverage, not regression tests. Additional test coverage be separate commits that precede refactoring to make it clear that a refactor does not change behavior.

timgraham · 2025-12-09T02:20:54Z

django_mongodb_backend/lookups.py

+                "tmp_name": {"$addToSet": expr.as_mql(compiler, connection, as_expr=True)},
            }
        },
+        {"$unionWith": {"pipeline": [{"$documents": [{"tmp_name": []}]}]}},


I guess this won't work with QE (but maybe the query isn't supported for some other reason).

A problem with our current testing setup is that we have to replicate all the tests we want to do with QE under the "encryption_" test app.

So if we have two different code paths in this method and under if self.wrap_for_global_aggregation: in query.py, we won't have very comprehensive testing.

Something to think about.

🤔 probably use some of the wrap ideas will work in order to provide support on QE, but I didn't explore it in deep. The current version with $facet isn't working either.

WaVEV · 2025-12-09T02:21:57Z

There is still some problem on evergreen / MongoDB 8:

ERROR: test_aggregation_default_using_duration_from_database (aggregation.tests.AggregateTestCase)
...
[2025/12/05 01:14:54.701]   File "/data/mci/6fa58471acf0c3d789a3318d07c79185/src/venv/lib/python3.10/site-packages/pymongo/helpers_shared.py", line 284, in _check_command_response
[2025/12/05 01:14:54.701]     raise OperationFailure(errmsg, code, response, max_wire_version)
[2025/12/05 01:14:54.701] pymongo.errors.OperationFailure: PlanExecutor error during aggregation :: caused by :: Builtin variable '$$NOW' is not available, full error: {'ok': 0.0, 'errmsg': "PlanExecutor error during aggregation :: caused by :: Builtin variable '$$NOW' is not available", 'code': 51144, 'codeName': 'Location51144'}

Do we need that workaround you had before?

mmh, well, the ticket is closed. So, maybe the image wasn't the appropriate? will try different images

timgraham · 2025-12-09T02:38:27Z

Do we need that workaround you had before?

mmh, well, the ticket is closed. So, maybe the image wasn't the appropriate? will try different images

Generally, I don't know if we can rely on users having the latest micro version, however, since the test is very obscure perhaps keeping CI green is enough for this case.

WaVEV · 2025-12-09T02:42:41Z

Do we need that workaround you had before?

mmh, well, the ticket is closed. So, maybe the image wasn't the appropriate? will try different images

Generally, I don't know if we can rely on users having the latest micro version, however, since the test is very obscure perhaps keeping CI green is enough for this case.

Right, so the workaround will be.

Jibola · 2025-12-09T17:53:34Z

django_mongodb_backend/fields/embedded_model_array.py

+            # $addToSet. The use of {_id: null} results in a
+            # single grouped array. However, because arrays from
+            # multiple documents are aggregated, the result is a
+            # list of lists.


If I understand correctly:
-> The project creates a list of documents based on sub-filer.
-> We unwind that list
-> We concatenate the list we unwound into one supreme list?
-> If list is empty we make it a bigger list
-> project only $tmp_name

can we rename tmp_name to something else to make it easier to follow? Maybe something like subquery_results?

Jibola · 2025-12-09T17:59:37Z

django_mongodb_backend/compiler.py

+            pipeline.append({"$group": {"_id": None, **group}})
+            # If there are no ids and no having clause, apply a global aggregation
+            self.wrap_for_global_aggregation = not bool(self.having)


Can I get an example of a query that would lead us here? It's harder to visualize all the way down. Not to say we shouldn't do this, but I think this necessitates a COLLSCAN.

Yeap, when the query does not have having

agg = Sum( Case(When(friends__age=40, then=F("friends__age"))), filter=Q(friends__name__startswith="test"), ) self.assertEqual(Author.objects.aggregate(age=agg)["age"], 80)

Extracted from django test.

WaVEV requested review from Jibola, aclark4life and timgraham November 20, 2025 04:30

aclark4life requested changes Nov 20, 2025

View reviewed changes

timgraham reviewed Nov 20, 2025

View reviewed changes

WaVEV force-pushed the INTPYTHON-833-Refactor-Remove-$facet-usage-in-aggregation-pipeline branch from 3e2c32d to 273957f Compare November 25, 2025 03:41

WaVEV force-pushed the INTPYTHON-833-Refactor-Remove-$facet-usage-in-aggregation-pipeline branch from 273957f to c082f0d Compare November 26, 2025 03:56

WaVEV changed the title ~~Remove $facet in top level group stages~~ INTPYTHON-833 Remove $facet in top level group stages Nov 26, 2025

timgraham reviewed Dec 2, 2025

View reviewed changes

WaVEV force-pushed the INTPYTHON-833-Refactor-Remove-$facet-usage-in-aggregation-pipeline branch from 9830b06 to 4dea72d Compare December 2, 2025 03:46

timgraham reviewed Dec 3, 2025

View reviewed changes

WaVEV commented Dec 5, 2025

View reviewed changes

WaVEV force-pushed the INTPYTHON-833-Refactor-Remove-$facet-usage-in-aggregation-pipeline branch from f65c7f1 to b8900af Compare December 5, 2025 05:37

timgraham reviewed Dec 9, 2025

View reviewed changes

WaVEV force-pushed the INTPYTHON-833-Refactor-Remove-$facet-usage-in-aggregation-pipeline branch 2 times, most recently from 702a518 to 598ef67 Compare December 9, 2025 16:38

Jibola reviewed Dec 9, 2025

View reviewed changes

WaVEV force-pushed the INTPYTHON-833-Refactor-Remove-$facet-usage-in-aggregation-pipeline branch from 598ef67 to 4929c30 Compare December 9, 2025 20:37

WaVEV added 3 commits December 9, 2025 20:21

Add coverage for overlap and in lookup in subqueries

d7276d7

Remove $facet from array output subqueries

4403f0a

Remove $facet when an ArrayField is used as rhs

4921adc

WaVEV force-pushed the INTPYTHON-833-Refactor-Remove-$facet-usage-in-aggregation-pipeline branch from c08dc58 to 4921adc Compare December 10, 2025 00:14

INTPYTHON-833 Remove $facet in top level group stages #449

Are you sure you want to change the base?

INTPYTHON-833 Remove $facet in top level group stages #449

Uh oh!

Conversation

WaVEV commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aclark4life commented Nov 20, 2025

Uh oh!

aclark4life left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jibola commented Nov 21, 2025

Uh oh!

timgraham commented Nov 25, 2025

Uh oh!

timgraham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WaVEV commented Dec 9, 2025

Uh oh!

timgraham commented Dec 9, 2025

Uh oh!

WaVEV commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WaVEV Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

WaVEV commented Nov 20, 2025 •

edited

Loading

WaVEV Dec 9, 2025 •

edited

Loading