-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Normalize Any to Contains instead of vice versa #30956
Conversation
} | ||
|
||
if (shaperExpression is ProjectionBindingExpression projectionBindingExpression) | ||
// Pattern-match Contains over ValuesExpression, translating to simplified 'item IN (1, 2, 3)' with constant elements |
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 pattern-matching code used to be in TrySimplifyValuesToInExpression, and called from TranslateAny/TranslateAll. Instead, normalization from Any/All to Contains now happens in preprocessing, so we know we end up here in TranslateContains when this is relevant.
@@ -210,44 +210,38 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp | |||
result); | |||
} | |||
|
|||
// Normalize x.Any(i => i == foo) to x.Contains(foo) |
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.
@maumar would appreciate a good look at this change... This removes all transformation of Contains to Any (below), and makes sure to always normalize Any/All to Contains when it's possible.
1b60b87
to
b4e9f25
Compare
selectExpression.ApplyProjection(); | ||
if (selectExpression.Limit == null | ||
&& selectExpression.Offset == null) | ||
var subquery = (SelectExpression)source.QueryExpression; |
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.
nit: same for TranslateAll above
{ | ||
var shaperExpression = shapedQueryExpression.ShaperExpression; | ||
// No need to check ConvertChecked since this is convert node which we may have added during projection | ||
if (shaperExpression is UnaryExpression { NodeType: ExpressionType.Convert } unaryExpression |
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.
nit: we use this exact pattern in several places, consider DRYing
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, you're right. I at least DRYed TryGetProjection within this visitor, but it's repeated e.g. in the PG visitor etc. Opened #31015 to track.
E.g. for easier pattern-matching of Contains
We have a translation code path for Queryable Contains, but that's dead code: replacing this code with an exception still passes all SQL Server functional tests. This is because at some point we started to normalize all Contains calls to Any in pre-processing.
Aside from the dead code, this forces us to pattern-match in translation in order to recognize Contains, which gets translated to specific patterns; this leads to a weird situation where we normalize out an important distinction, and then have to re-recognize it. This is becoming more problematic with the recent changes to how we deal with Contains. We should do the opposite and make sure that Any is normalized to Contains.
Note that this is an infrastructure 1st step PR... TranslateContains still delegates to TranslateAny if it can't pattern-match Contains over inline collection, so we end up in a similar place. #30955 will build on top of this to translate to InExpression with subquery, as well as some other planned work.
Note that this also removes quite a lot of useless parentheses around NOT EXISTS :)