-
Notifications
You must be signed in to change notification settings - Fork 29
INTPYTHON-739: (Addendum) Support converting $expr with $getField paths #392
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.
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
base: main
Are you sure you want to change the base?
Conversation
b99aba3
to
94eefad
Compare
expr = { | ||
"$expr": { | ||
"$gt": [ | ||
{"$getField": {"input": "$price", "field": "value"}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we optimize this down to remove the $getField
when the field names don't contain $
or .
? Something like:
{
"$match": {
"$expr": {
"$gt": ["$price.value", "$discounted_price.value"]
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could. I'm trying to think if there's any case where this would be an issue if it mistakenly resolved it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would have to be a two-phase conversion. I think it may even be best to still change it in our actual query code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming here that this change ends up being needed. Holding off until we either get customer complaints or the refactor can't solve this makes more sense to me for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I tried to make it work (and I think I managed to), but the silent mutation of $getField
to $field
is worrying to me. Even if I manage to make things pass tests, it just feels out of scope of the optimizers goal, which is to explicitly change things from $expr
to $match
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements support for $getField
expressions in the expression converter system, allowing MongoDB aggregation expressions using $getField
to be optimized into simpler query conditions.
- Adds helper methods to detect and convert
$getField
expressions to dot notation paths - Updates binary and
$in
operators to handle$getField
expressions alongside simple field references - Adds comprehensive test coverage for
$getField
conversion scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
django_mongodb_backend/query_conversion/expression_converters.py | Core implementation of $getField support with path conversion methods and updated operator handling |
tests/expression_converter_/test_op_expressions.py | Test cases for $getField conversion across all binary operators and $in operator |
tests/expression_converter_/test_match_conversion.py | Integration tests for $getField optimization in match expressions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
# Check if first argument is a simple field reference | ||
# Check if second argument is a list of simple values | ||
if (field_name := cls.convert_path_name(field_expr)) and ( | ||
isinstance(values, list | tuple | set) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use explicit tuple syntax (list, tuple, set)
instead of union operator |
for better compatibility with older Python versions.
isinstance(values, list | tuple | set) | |
isinstance(values, (list, tuple, set)) |
Copilot uses AI. Check for mistakes.
Summary
An addendum to INTPYTHON-739 This PR implements support for
$getField
expressions in the expression converter system, allowing MongoDB aggregation expressions using$getField
to be optimized into simpler query conditions. This is especially useful when optimizing queries against embedded model fields.The format of
$getField
calls need to meet these conditions:This supports nested
$getField
operations.Changes in this PR
Test Plan
python manage.py shell
See an example query below.Screenshots (grabbed text)
Callouts
$getField
call unless they also feed into a$match
conversion because silently mutating$getField
should best be dealt in the larger refactor.NoneType
is used a lookup, the optimization needs to wrap two statements in an$and
query.