-
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
Query: Improve the logic of null expansion in join conditions with an… #7826
Conversation
58de9f1
to
60bba22
Compare
Updated. |
&& expressionType == ExpressionType.Equal) | ||
{ | ||
var translatedExpression = TransformNullComparison(leftExpressions[0], rightExpressions[0], binaryExpression.NodeType) | ||
?? Expression.MakeBinary(expressionType, leftExpressions[0], rightExpressions[0]); |
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 ExpressionType.Equal rather than expressionType here, just so its more clear
{ | ||
var translatedExpression = TransformNullComparison(leftExpressions[0], rightExpressions[0], binaryExpression.NodeType) | ||
?? Expression.MakeBinary(expressionType, leftExpressions[0], rightExpressions[0]); | ||
return Expression.AndAlso(translatedExpression, Expression.Constant(true, translatedExpression.Type)); |
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.
Expression.Constant(true), since translatedExpression is guaranteed to be bool
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 actually forgot to submit updated. Will fix in future PR.
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 TranslatedExpression
is guaranteed to return bool type?
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.
updated in #7843
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.
It should be but it's not that clear. I guess it is safer to leave it as is after all
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 concluded this way:
TransformNullComparison returns either IsNullExpression or Not(IsNullExpression).
IsNullExpression as type hardcoded to bool. Not expression takes type from the inner expression so it have to bool only in this case.
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.
…onymous types
@maumar @anpete
Due to the way LINQ processes
equals
with normal properties comparison & anonymous type comparison.