Improves filtering by enum #193
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- non-nullable + nullable enum Fields - check for cast existance
+ new formatting applied
AlexUstinov
reviewed
Nov 19, 2021
| if (b.Left.StripCasts().Type.StripNullable().IsEnum | ||
| && b.Right.StripCasts().NodeType == ExpressionType.Constant) { | ||
|
|
||
| var rawEnum = b.Left.StripCasts(); |
There was a problem hiding this comment.
That one is used in the if condition above so it makes sense to move the variable declaration before the if statement.
There was a problem hiding this comment.
I'd even declared two variables
var rawLeftEnum = b.Left.StripCasts();
var rawRightEnum = b.Right.StripCasts();or even four
var rawLeftEnum = b.Left.StripCasts();
var rawLeftEnumType = rawLeftEnum.Type;
var rawRightEnum = b.Right.StripCasts();
var rawRightEnumType = rawRightEnum.Type;that way we could avoid multiple excessive method and property getter calls.
There was a problem hiding this comment.
yes, thank you
Orm/Xtensive.Orm/Orm/Building/Builders/PartialIndexFilterBuilder.cs
Outdated
Show resolved
Hide resolved
| @@ -207,6 +207,30 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression) | |||
| right = Visit(binaryExpression.Right); | |||
| } | |||
| } | |||
| // Following two checks for enums are here to improve result query | |||
| // performance because they let not to cast columns to integer. | |||
| else if (EnumRewritableOperations(binaryExpression) | |||
There was a problem hiding this comment.
Why not just have a single else if statement for the EnumRewritableOperations case? In that case, you could declare four variables as I suggested for the PartialIndexFilterBuilder inside that else if block. The cost of that is a duplicated block
else {
left = Visit(binaryExpression.Left);
right = Visit(binaryExpression.Right);
}but as for me, it is worth it.
Also please pay attention to the EnumRewritableOperations method implementation. We can avoid allocations there.
+ renames in PartialIndexFilterBuilder
AlexUstinov
reviewed
Nov 22, 2021
| ? rawEnum.Type.StripNullable().GetEnumUnderlyingType().ToNullable() | ||
| : rawEnum.Type.GetEnumUnderlyingType(); | ||
| if (bareLeftType.IsEnum && bareRight.NodeType == ExpressionType.Constant) { | ||
| var typeToCast = bareLeft.Type.IsNullable() |
There was a problem hiding this comment.
Still accessing the Type property here.
| if (bareLeftType.IsEnum && bareRight.NodeType == ExpressionType.Constant) { | ||
| var typeToCast = bareLeft.Type.IsNullable() | ||
| ? bareLeftType.GetEnumUnderlyingType().ToNullable() | ||
| : bareLeft.Type.GetEnumUnderlyingType(); |
| ? rawEnum.Type.StripNullable().GetEnumUnderlyingType().ToNullable() | ||
| : rawEnum.Type.GetEnumUnderlyingType(); | ||
| else if (bareRightType.IsEnum && bareLeft.NodeType == ExpressionType.Constant) { | ||
| var typeToCast = bareRight.Type.IsNullable() |
| else if (bareRightType.IsEnum && bareLeft.NodeType == ExpressionType.Constant) { | ||
| var typeToCast = bareRight.Type.IsNullable() | ||
| ? bareRightType.GetEnumUnderlyingType().ToNullable() | ||
| : bareRight.Type.GetEnumUnderlyingType(); |
| var bareRightType = bareRight.Type.StripNullable(); | ||
|
|
||
| if (bareLeftType.IsEnum && bareRight.NodeType == ExpressionType.Constant) { | ||
| var typeToCast = bareLeft.Type.IsNullable() |
| if (bareLeftType.IsEnum && bareRight.NodeType == ExpressionType.Constant) { | ||
| var typeToCast = bareLeft.Type.IsNullable() | ||
| ? bareLeftType.GetEnumUnderlyingType().ToNullable() | ||
| : bareLeft.Type.GetEnumUnderlyingType(); |
| right = Visit(Expression.Convert(binaryExpression.Right, typeToCast)); | ||
| } | ||
| else if (bareRightType.IsEnum && bareLeft.NodeType == ExpressionType.Constant) { | ||
| var typeToCast = (bareRight.Type.IsNullable()) |
| else if (bareRightType.IsEnum && bareLeft.NodeType == ExpressionType.Constant) { | ||
| var typeToCast = (bareRight.Type.IsNullable()) | ||
| ? bareRightType.GetEnumUnderlyingType().ToNullable() | ||
| : bareRight.Type.GetEnumUnderlyingType(); |
AlexUstinov
approved these changes
Nov 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #189
In certain cases Translator changes binary expression to make following operations not add cast of column to real underlying type of the
enumconstants. Basically, it works around C# compiler feature of using integer constants instead of smaller types constants e.gbyteconstants.