Avoid using ^ and ~ when invalid because of value converters
#35124
Conversation
There was a problem hiding this comment.
@ranma42 thanks for working on this and sorry it took a while to review (lots of workload at the moment post-9.0!).
See my two comments and let me know what you think... FYI the deadline for getting a fix into 9.0.1 is extremely close (ideally we'd have this finished today). If you can, please let me know if you don't have time to iterate over this right away - I'm happy to take over and do changes.
Once we're good here I'll also prepare a backport PR for 9.0.1.
| if (!inSearchConditionContext | ||
| && (newLeft.Type == typeof(bool) || newLeft.Type.IsEnum || newLeft.Type.IsInteger()) | ||
| && (newRight.Type == typeof(bool) || newRight.Type.IsEnum || newRight.Type.IsInteger())) | ||
| && (leftType == typeof(bool) || leftType.IsEnum || leftType.IsInteger()) |
There was a problem hiding this comment.
I think the IsEnum case can be removed, since now that we're looking at the provider type is should never be an enum (enums are value-converted to int)
| @@ -265,7 +267,9 @@ protected virtual Expression VisitSqlUnary(SqlUnaryExpression sqlUnaryExpression | |||
| case ExpressionType.Not when sqlUnaryExpression.Type == typeof(bool): | |||
| { | |||
| // when possible, avoid converting to/from predicate form | |||
| if (!inSearchConditionContext && sqlUnaryExpression.Operand is not (ExistsExpression or InExpression or LikeExpression)) | |||
| if (!inSearchConditionContext && | |||
| sqlUnaryExpression.TypeMapping?.Converter is null && | |||
There was a problem hiding this comment.
Isn't a more correct fix here to look at sqlUnaryExpression.TypeMapping?.Converter?.ProviderClrType ?? sqlUnaryExpression.Type above (in the when comparing to typeof(bool))? For example, if there's a Y/N string being value converted to BIT, I think we do want this code to go through and do OnesComponent (on the database side it's just a BIT).
There was a problem hiding this comment.
yup, I just thought that it was not a very likely scenario; will update ASAP
There was a problem hiding this comment.
You'd be surprised what people do 🤣
There was a problem hiding this comment.
I am not even sure if it is possible to build an expression in which a y/n string is being negated 🤔
I applied your suggestion, but I don't have time to add a test for this case right now (I can work on it in the weekend, if needed).
There was a problem hiding this comment.
You're generally completely right... Though I'll point out that Expression.Not() has an overload which accepts an implementing method - this allows arbitrary types to be "negatable". Similarly, IIRC user types which implement the not operator via operator overloading should also be supported (by am no longer sure about that).
Of course, it could be argued that EF shouldn't translate negation for those cases (where the implementing method/operator obviously are not taken into account)... But this is all indeed highly academic. I also don't think a test is necesary here at this point.
|
I will try to push the changes tonight (aka in ~4 hours) |
The transformation of equality/in-equality in a (negated) XOR is only possible when the expressions are BIT or integer types on the SQL side (i.e. taking value conversion into account). Similarly, the Boolean negation `NOT` can be implemented as `~` only if the underlying expression is a BIT. Fixes dotnet#35093.
…net#35124) The transformation of equality/in-equality in a (negated) XOR is only possible when the expressions are BIT or integer types on the SQL side (i.e. taking value conversion into account). Similarly, the Boolean negation `NOT` can be implemented as `~` only if the underlying expression is a BIT. Fixes dotnet#35093. (cherry picked from commit e6abfdd)
|
Backport PR: #35241 |
) (#35241) The transformation of equality/in-equality in a (negated) XOR is only possible when the expressions are BIT or integer types on the SQL side (i.e. taking value conversion into account). Similarly, the Boolean negation `NOT` can be implemented as `~` only if the underlying expression is a BIT. Fixes #35093. (cherry picked from commit e6abfdd) Co-authored-by: Andrea Canciani <[email protected]>
The transformation of equality/in-equality in a (negated) XOR is only possible
when the expressions are BIT or integer types on the SQL side (i.e. taking value
conversion into account).
Similarly, the Boolean negation
NOTcan be implemented as~only if theunderlying expression is a BIT.
Fixes #35093.