INTPYTHON-736 Convert simple $expr queries to $match queries #373

Jibola · 2025-08-21T21:00:56Z

Summary

This PR introduces an abstraction to convert some of our simple $expr queries to $match queries without $expr in them. If the $expr query cannot be converted to a $match query, it will leave it as a $expr. This behavior is nested. Currently this is limited to $eq and $in operations for now, but in the future can be extended to other $expr conversions.

This only affects the querying done in self.match_mql as the querying for lookups, groupings, or aggregation pipelines are out of scope for this pr.

Changes in this PR

Introduce the QueryOptimizer abstraction (which can be changed; it doesn't store state so it does not need to remain an object)
Introduced ExpressionConverters which house the logic for converting queries from $expr usage to their $match equivalents.
Added several tests to confirm the behavior of the $eq, $in, $and, $or in drilling down of expression conversion.

Test Plan

Manual testing through python manage.py shell See an example query below.
Added test cases. Some old tests will fail and need to be fixed

Screenshots (grabbed text)

# Before
>>> Author.objects.filter(author_city__in=["London"])
{
    "$match": {
        "$expr": {
            "$and": [
                "$in": ["$author_city", "London"]
            ]
        }
    }
}

# After 
>>> Author.objects.filter(author_city__in=["London"])
{
    "$match": {
        "$and": {
            "$in": ["$author_city", "London"]
        }
    }
}

Callouts

This is still a proof of concept. Like said above, QueryOptimizer should be removed and left as its functions if it is not going to retain state. Initialization is unnecessary.
This will not work on Embedded Models until a$getField converter is made.
This PR only assumes self.match_mql resolves to a $expr (which is true) future work should include keeping any query that doesn't use self.match_mql.

NoahStapp · 2025-08-29T15:23:38Z

The remaining test failure test_changelist_view_list_editable_changed_objects_uses_filter is fixed by mongodb-forks/django#17.

tests/expression_converter_/models.py

ShaneHarvey · 2025-09-04T17:20:56Z

django_mongodb_backend/query_conversion/expression_converters.py

@@ -0,0 +1,129 @@
+"""Expression To Match Converters"""


Wow a file with no imports is a rare gem.

django_mongodb_backend/query_conversion/expression_converters.py

ShaneHarvey · 2025-09-04T17:23:36Z

django_mongodb_backend/query_conversion/expression_converters.py

+            return False
+        if isinstance(value, list | tuple | set):
+            return all(cls.is_simple_value(v) for v in value)
+        # TODO: Expand functionality to support `$getField` conversion


Is this TODO for this PR or do we need a new tracking ticket?

This would be a new tracking ticket, but we can table that until we determine if our new $expr generation refactor will handle this case.

ShaneHarvey · 2025-09-04T17:25:29Z

django_mongodb_backend/query_conversion/expression_converters.py

+
+
+class _EqExpressionConverter(_BaseExpressionConverter):
+    """Convert $eq operation to a $match compatible query."""


For this classes it would be helpful to include small examples of what the transformation before+after looks like. Is that possible?

Sure, I can add those.

timgraham · 2025-09-04T18:50:16Z

tests/expression_converter_/test_op_expressions.py

+    def test_non_dict_expression(self):
+        expr = ["$status", "active"]
+        self.assertNotOptimizable(expr)
+
+    def test_empty_dict_expression(self):
+        expr = {}
+        self.assertNotOptimizable(expr)
+
+    def test_non_convertible(self):
+        expr = {"$gt": ["$price", 100]}
+        self.assertNotOptimizable(expr)
+
+    def _test_conversion_various_types(self, conversion_test):
+        for _type, val in self.CONVERTIBLE_TYPES.items():
+            with self.subTest(_type=_type, val=val):
+                conversion_test(val)


Tests shouldn't appear in a test case since they'll be run redundantly on each subclass.

Do you mean tests shouldn't appear in a test case parent class?

Yes, at least for Django tests "TestCase" means a class that's inherited from (SimpleTestCase, TestCase, LiveServerTestCase, etc.).

timgraham · 2025-09-04T18:51:12Z

tests/expression_converter_/test_op_expressions.py

+from django_mongodb_backend.query_conversion.expression_converters import convert_expression
+
+
+class TestBaseExpressionConversionCase(SimpleTestCase):


Suggested change

class TestBaseExpressionConversionCase(SimpleTestCase):

class ExpressionConversionTestCase(SimpleTestCase):

aclark4life · 2025-09-04T20:00:43Z

I'd feel slightly better about this PR if the code was in the existing query_utils or if we anticipate future growth, make the query module a package.

NoahStapp · 2025-09-04T20:07:26Z

I'd feel slightly better about this PR if the code was in the existing query_utils or if we anticipate future growth, make the query module a package.

Is there a strong reason to move it into query_utils? Adding a new folder we can cleanly remove once the generation refactor is in looks cleaner to me.

ShaneHarvey · 2025-09-04T20:26:18Z

tests/expression_converter_/test_match_conversion.py

+            "$expr": {
+                "$and": [
+                    {"$eq": ["$status", "active"]},
+                    {"$gt": ["$price", 100]},  # Not optimizable


Why not? "$gt" is valid in a find() query so it should be valid in a $match as well.

This is actually a good point of discussion: we can add additional operators ($gt, $gte, $lt, $lte are all simple) but each is additional code + tests we need to add. If we expect this to be a short-term solution we replace, is it worth expanding the scope to include those operators?

Added support for those four operators.

I think this comment answers that question, at least in part. Leaving aside the code layout, I'd be inclined to treat this PR like an improvement I have no specific expectations about and would try to fix as much as I could in it. This is the "framework approach" we talked about in office hours when folks said that a framework for these optimizations was not the goal, but I think you could treat it as such if the trade-off between disposable features and your time and energy seems reasonable at the start.

The work to add them was minimal, but I'd push back against adding any others that will require more complex code changes. Covering the basic operators we do here should provide a significant speedup for most users until we can rewrite our query generation entirely.

aclark4life · 2025-09-04T20:46:16Z

I'd feel slightly better about this PR if the code was in the existing query_utils or if we anticipate future growth, make the query module a package.

Is there a strong reason to move it into query_utils? Adding a new folder we can cleanly remove once the generation refactor is in looks cleaner to me.

Until we hear some feedback from @WaVEV or others about the proposed refactor, I would take @Jibola 's PoC at face value. We know it implements significant performance improvements and we know he stated's that the QueryOptimizer class was an abstraction for convenience in developing the PoC.

As such, I would take the functions in that class and move them to query_utils (assuming that's not a heavy lift) and proceed as if we don't know when these converters will be removed, if ever.

Also even if the refactor timeline is short, the need to resolve the performance issue is shorter and I think we should proceed as best we can with what we know now and exclude things we don't know yet from that decision making process. To me, that means implementing the PoC the same way we'd implement any other feature, without the expectation that we're going to be removing it in the next release.

That said, I don't feel that strongly about it. I'm probably going to remain skeptical about the refactor's ability to improve performance in a similar way, but I look forward to seeing it happen!

timgraham · 2025-09-04T21:05:38Z

I'm with Noah for now. It's better to keep this code separate than to mix it up with other things in query_utils. I might lightly restructure it when I take a deeper look later.

… for $expr removal for mongodb/django-mongodb-backend#373

ShaneHarvey · 2025-09-05T17:15:08Z

django_mongodb_backend/query_conversion/expression_converters.py

        return None


+class _GtExpressionConverter(_BaseExpressionConverter):


These classes are all nearly identical. Could we refactor them with a common base class like this:

class _GtExpressionConverter(BinaryExpression): operator = "$gt" class _GtExpressionConverter(BinaryExpression): operator = "$gte" class _LtExpressionConverter(BinaryExpression): operator = "$lt" ...

Done. Added $eq as well with a tiny branch.

timgraham · 2025-09-05T18:44:14Z

tests/queries_/test_mql.py


 class FKLookupConditionPushdownTests(TestCase):
    def test_filter_on_local_and_related_fields(self):
+        self.maxDiff = None


maxDiff is a class attribute and I believe mutating it in one test modifiest for all subsequent tests in the class.

Either we could set it on each class level, add it to MongoTestCaseMixin, or omit it and only add it when debugging tests locally.

timgraham · 2025-09-05T18:45:37Z

django_mongodb_backend/query_conversion/expression_converters.py

+    """Convert $eq operation to a $match compatible query.
+
+    For example::
+        "$expr": {
+                {"$eq": ["$status", "active"]}
+        }
+    is converted to::
+        {"status": "active"}
+    """


I think the example could be on the base class so we don't need to repeat it on every class with only the operator replaced.

timgraham · 2025-09-05T19:19:35Z

django_mongodb_backend/query_conversion/query_optimizer.py

+        Returns:
+            List of optimized match conditions
+        """
+        expr_query = deepcopy(expr)


This was part of Jib's initial commit, but I'm not sure why copying would be needed. I spot checked some tests and didn't see any failures. Could try a CI run without it?

WaVEV · 2025-09-06T23:31:27Z

django_mongodb_backend/query_conversion/expression_converters.py

+        if isinstance(value, (list, tuple, set)):  # noqa: UP038
+            return all(cls.is_simple_value(v) for v in value)
+        # TODO: Expand functionality to support `$getField` conversion
+        return not isinstance(value, dict) or value is None


🤔 I think this could be rewritten as:

Suggested change

return not isinstance(value, dict) or value is None

return not isinstance(value, dict)

not is instance(value, dict) implies value isn't None. and if value is a dict isn't None 😅

I'll check for None at the start of the method since isinstance() is comparatively expensive.

WaVEV · 2025-09-07T00:14:49Z

django_mongodb_backend/query_conversion/expression_converters.py

+        if isinstance(combined_conditions, list):
+            optimized_conditions = []
+            for condition in combined_conditions:
+                if isinstance(condition, dict) and len(condition) == 1:


What if some conditions pass the if and others don't

I think isn't possible 🤔 . The way that the queries are made any literal like True or False are handled by a dict. Is there any condition/operator that needs more than one key?

Emanuel, I'm not sure exactly what you meant here. Feel free to open a follow up PR.

WaVEV

LGTM.

WaVEV · 2025-09-07T00:40:14Z

django_mongodb_backend/query_conversion/expression_converters.py

+    "$gt": _GtExpressionConverter,
+    "$gte": _GteExpressionConverter,
+    "$lt": _LtExpressionConverter,
+    "$lte": _LteExpressionConverter,


I think we could covers not with $nor.

Co-authored-by: Noah Stapp <noah.stapp@mongodb.com>

… for $expr removal for mongodb/django-mongodb-backend#373

Jibola changed the title ~~PoC Expression Conversion Abstraction~~ [INTPYTHON-TODO]: PoC Expression Conversion Abstraction Aug 25, 2025

NoahStapp changed the title ~~[INTPYTHON-TODO]: PoC Expression Conversion Abstraction~~ INTPYTHON-736 - Convert simple $expr queries to $match queries Aug 27, 2025

NoahStapp marked this pull request as ready for review September 4, 2025 15:47

NoahStapp requested review from ShaneHarvey and timgraham September 4, 2025 15:47

ShaneHarvey reviewed Sep 4, 2025

View reviewed changes

NoahStapp requested review from ShaneHarvey and WaVEV September 4, 2025 18:09

timgraham reviewed Sep 4, 2025

View reviewed changes

NoahStapp requested review from aclark4life and timgraham September 4, 2025 19:17

ShaneHarvey reviewed Sep 4, 2025

View reviewed changes

timgraham mentioned this pull request Sep 5, 2025

INTPYTHON-736 - Update test to not expect $expr mongodb-forks/django#17

Closed

timgraham added a commit to timgraham/django that referenced this pull request Sep 5, 2025

Update test_changelist_view_list_editable_changed_objects_uses_filter…

23de328

… for $expr removal for mongodb/django-mongodb-backend#373

NoahStapp requested a review from ShaneHarvey September 5, 2025 14:36

ShaneHarvey requested changes Sep 5, 2025

View reviewed changes

NoahStapp requested a review from ShaneHarvey September 5, 2025 17:29

ShaneHarvey approved these changes Sep 5, 2025

View reviewed changes

aclark4life approved these changes Sep 5, 2025

View reviewed changes

timgraham reviewed Sep 5, 2025

View reviewed changes

WaVEV reviewed Sep 6, 2025

View reviewed changes

timgraham changed the title ~~INTPYTHON-736 - Convert simple $expr queries to $match queries~~ INTPYTHON-736 Convert simple $expr queries to $match queries Sep 7, 2025

WaVEV reviewed Sep 7, 2025

View reviewed changes

WaVEV approved these changes Sep 7, 2025

View reviewed changes

WaVEV reviewed Sep 7, 2025

View reviewed changes

timgraham force-pushed the expr-conversion-poc branch 2 times, most recently from 0c27833 to b99aba3 Compare September 8, 2025 11:41

INTPYTHON-736 Convert simple $expr queries to $match queries

94eefad

Co-authored-by: Noah Stapp <noah.stapp@mongodb.com>

timgraham force-pushed the expr-conversion-poc branch from b99aba3 to 94eefad Compare September 8, 2025 11:44

timgraham approved these changes Sep 8, 2025

View reviewed changes

timgraham merged commit 94eefad into main Sep 8, 2025
19 checks passed

timgraham deleted the expr-conversion-poc branch September 8, 2025 12:21

Jibola mentioned this pull request Sep 11, 2025

INTPYTHON-749: Fix bug in null matching on queryoptimizer #401

Closed

timgraham added a commit to mongodb-forks/django that referenced this pull request Oct 21, 2025

Update test_changelist_view_list_editable_changed_objects_uses_filter…

c76b17d

… for $expr removal for mongodb/django-mongodb-backend#373



		class _EqExpressionConverter(_BaseExpressionConverter):
		"""Convert $eq operation to a $match compatible query."""

		from django_mongodb_backend.query_conversion.expression_converters import convert_expression


		class TestBaseExpressionConversionCase(SimpleTestCase):

	class TestBaseExpressionConversionCase(SimpleTestCase):
	class ExpressionConversionTestCase(SimpleTestCase):

		return None


		class _GtExpressionConverter(_BaseExpressionConverter):

	return not isinstance(value, dict) or value is None
	return not isinstance(value, dict)

INTPYTHON-736 Convert simple $expr queries to $match queries #373

INTPYTHON-736 Convert simple $expr queries to $match queries #373

Uh oh!

Conversation

Jibola commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes in this PR

Test Plan

Screenshots (grabbed text)

Callouts

Uh oh!

NoahStapp commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

aclark4life commented Sep 4, 2025

Uh oh!

NoahStapp commented Sep 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aclark4life commented Sep 4, 2025

Uh oh!

timgraham commented Sep 4, 2025

Uh oh!

ShaneHarvey Sep 5, 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!

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!

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!

WaVEV left a comment • 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!

Jibola commented Aug 21, 2025 •

edited

Loading

NoahStapp commented Aug 29, 2025 •

edited

Loading

ShaneHarvey Sep 5, 2025 •

edited

Loading

WaVEV left a comment •

edited

Loading