-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Fix](Nereids) fix infer predicate lost cast of source expression #23692
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
Conversation
|
run buildall |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
| return expr.rewriteUp(e -> { | ||
| if (isDataTypeValid(originDataType, leftSlotEqualToRightSlot)) { | ||
| if (ExpressionUtils.isTwoExpressionEqualWithCast(e, leftSlotEqualToRightSlot.child(0))) { | ||
| if (!leftSlotEqualToRightSlot.child(1).getDataType().equals(e.getDataType())) { |
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 some ut case please~
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
0680917 to
fd79201
Compare
|
run buildall |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
| if (isDataTypeValid(originDataType, leftSlotEqualToRightSlot)) { | ||
| if (ExpressionUtils.isTwoExpressionEqualWithCast(e, leftSlotEqualToRightSlot.child(0))) { | ||
| if (ExpressionUtils.isTwoExpressionEqualWithCast(e, leftSlotEqualToRightSlot.child(0)) | ||
| && !(e instanceof Cast)) { |
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.
why add !(e instance Cast) ? add some comment to explain it.
btw, !(e instance Cast) is a common predicate, should put at L106.
and if (isDataTypeValid(originDataType, leftSlotEqualToRightSlot)) could put before return?
and, we do not use the second parameters at all in isDataTypeValid at 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.
done
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.
because when rewriteup, it would do rewrite repeatly, assume we have a cast(a) = constant,and a = b for example. When rewrite up, it would replace a to b first and then replace cast(b) -> a when go back to cast layer. isDatatypeValid parameter has been used.
fd79201 to
f0be295
Compare
|
run buildall |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
|
run buildall |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
…3692) Problem: When inferring predicate,we lost cast of source expressions and some datatype derivation. Example: a = b and cast(a as targetType) = constant (cast(a as targetType) = constant ) this expression is define as source expression. we expect getting cast(b as targetType) = constant instead of b = constant Reason: When inferring predicate, we will compare original type of a and b. if they can be cast without precision lost, a new predicate would be created. But created predicate forgot to cast to target type Solved: Add cast to target type, and open make other datatype valid also.
…3692) Problem: When inferring predicate,we lost cast of source expressions and some datatype derivation. Example: a = b and cast(a as targetType) = constant (cast(a as targetType) = constant ) this expression is define as source expression. we expect getting cast(b as targetType) = constant instead of b = constant Reason: When inferring predicate, we will compare original type of a and b. if they can be cast without precision lost, a new predicate would be created. But created predicate forgot to cast to target type Solved: Add cast to target type, and open make other datatype valid also.
…sion (apache#23692)" This reverts commit be36183.
…sion (apache#23692)" (apache#25008) This reverts commit be36183.
|
refactored in other way |
Proposed changes
Problem:
When inferring predicate,we lost cast of source expressions and some datatype derivation.
Example:
a = b and cast(a as targetType) = constant
(cast(a as targetType) = constant ) this expression is define as source expression.
we expect getting cast(b as targetType) = constant instead of b = constant
Reason:
When inferring predicate, we will compare original type of a and b. if they can be cast
without precision lost, a new predicate would be created. But created predicate forgot
to cast to target type
Solved:
Add cast to target type, and open make other datatype valid also.
Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...