-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
CSHARP-5436: Optimize special case of Any with constant array and fie… #1585
base: main
Are you sure you want to change the base?
Conversation
…ld equals parameter in predicate
@@ -103,6 +103,50 @@ static bool OperatorMapsNullToNull(AstUnaryOperator @operator) | |||
} | |||
} | |||
|
|||
public override AstNode VisitExprFilter(AstExprFilter node) | |||
{ | |||
var optimizedNode = (AstExprFilter)base.VisitExprFilter(node); |
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.
Decided to visit upfront, for easier handling of nested documents scenario.
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.
Is it safe to assume base.VisitExprFilter
always returns an AstExprFilter
? I guess looking at the source code we can convince ourselves that it does.
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.
Good catch. Even if it always returns VisitExprFilter for now, it's better to double-check.
mapExpression.In is AstBinaryExpression inBinaryExpression && | ||
inBinaryExpression.Operator == AstBinaryOperator.Eq && | ||
TryGetBinaryExpressionArguments(inBinaryExpression, out AstFieldPathExpression fieldPathExpression, out AstVarExpression varExpression) && | ||
fieldPathExpression.Path.Length > 1 && fieldPathExpression.Path[0] == '$' && fieldPathExpression.Path[1] != '$' && |
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.
Should we have some more elegant way to check if fieldPath points to the "current" variable?
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.
Not sure what you mean.
But anything that starts with $$
is not a field so I think that's all we need to check.
|
||
return optimizedNode; | ||
|
||
static bool TryGetBinaryExpressionArguments<T1, T2>(AstBinaryExpression binaryExpression, out T1 arg1, out T2 arg2) |
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.
Theoretically could be reusable.
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 can wait until there is a need.
@@ -103,6 +103,50 @@ static bool OperatorMapsNullToNull(AstUnaryOperator @operator) | |||
} | |||
} | |||
|
|||
public override AstNode VisitExprFilter(AstExprFilter node) | |||
{ | |||
var optimizedNode = (AstExprFilter)base.VisitExprFilter(node); |
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.
Is it safe to assume base.VisitExprFilter
always returns an AstExprFilter
? I guess looking at the source code we can convince ourselves that it does.
fieldPathExpression.Path.Length > 1 && fieldPathExpression.Path[0] == '$' && fieldPathExpression.Path[1] != '$' && | ||
varExpression == mapExpression.As) | ||
{ | ||
return AstFilter.In(AstFilter.Field(fieldPathExpression.Path.Substring(1)), inputArrayValue); |
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.
Add a comment just before this line to visualize the simplification being done:
// { $expr : { $anyElementTrue : { $map : { input : <constantArray>, as : "<var>", in : { $eq : ["$<dottedFieldName>", "$$<var>"] } } } } }
// => { "<dottedFieldName>" : { $in : <constantArray> } }
return AstFilter.In(AstFilter.Field(fieldPathExpression.Path.Substring(1)), inputArrayValue);
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.
Done
mapExpression.In is AstBinaryExpression inBinaryExpression && | ||
inBinaryExpression.Operator == AstBinaryOperator.Eq && | ||
TryGetBinaryExpressionArguments(inBinaryExpression, out AstFieldPathExpression fieldPathExpression, out AstVarExpression varExpression) && | ||
fieldPathExpression.Path.Length > 1 && fieldPathExpression.Path[0] == '$' && fieldPathExpression.Path[1] != '$' && |
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.
Not sure what you mean.
But anything that starts with $$
is not a field so I think that's all we need to check.
|
||
return optimizedNode; | ||
|
||
static bool TryGetBinaryExpressionArguments<T1, T2>(AstBinaryExpression binaryExpression, out T1 arg1, out T2 arg2) |
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 can wait until there is a need.
|
||
namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira | ||
{ | ||
public class AnyEqualTests : Linq3IntegrationTest |
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.
Is this the file name we want for these tests?
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.
Nice work! LGTM.
…ld equals parameter in predicate