-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix](unique function) add between back for fix unique function #55407
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
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
4 similar comments
|
run buildall |
|
run buildall |
|
run buildall |
|
run buildall |
TPC-H: Total hot run time: 34803 ms |
TPC-DS: Total hot run time: 187196 ms |
ClickBench: Total hot run time: 32.97 s |
|
run buildall |
TPC-H: Total hot run time: 34390 ms |
TPC-DS: Total hot run time: 187413 ms |
ClickBench: Total hot run time: 32.49 s |
|
run p0 |
2 similar comments
|
run p0 |
|
run p0 |
|
run buildall |
e9ea359 to
c469e06
Compare
c469e06 to
c97458f
Compare
|
run buildall |
TPC-H: Total hot run time: 34427 ms |
TPC-DS: Total hot run time: 187103 ms |
ClickBench: Total hot run time: 33.16 s |
FE Regression Coverage ReportIncrement line coverage |
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
### What problem does this PR solve? If an unique function exists multiple times, then be will calculate it multiple times for each row, so it will be error. for example: `filter(random() between 10 and 20)`, after rewrite the `between`, it will get `filter(random() >= 10 and random() <= 20)`, this will contains two random in one expression, the two RANDOM should be one, so we add a project, then we can get `filter(k >= 10 and k <= 20) -> project(random() as k)` this PR also fix BETWEEN expression bug introduced by #55407 - between shouldn't be PropagateNullable because FoldConstantRuleOnFE will rewrite a propagate nullable expression to null if any children is NULL, but for sql `10 between null and 5` should be `FALSE`, not `NULL`; - after analyzed between expression, it will get an AND expression, then anlyzed join other conjunctions, need to extract conjuncts of each analyzed other conjunction (that is flattern AND).
### What problem does this PR solve? If an unique function exists multiple times, then be will calculate it multiple times for each row, so it will be error. for example: `filter(random() between 10 and 20)`, after rewrite the `between`, it will get `filter(random() >= 10 and random() <= 20)`, this will contains two random in one expression, the two RANDOM should be one, so we add a project, then we can get `filter(k >= 10 and k <= 20) -> project(random() as k)` this PR also fix BETWEEN expression bug introduced by apache#55407 - between shouldn't be PropagateNullable because FoldConstantRuleOnFE will rewrite a propagate nullable expression to null if any children is NULL, but for sql `10 between null and 5` should be `FALSE`, not `NULL`; - after analyzed between expression, it will get an AND expression, then anlyzed join other conjunctions, need to extract conjuncts of each analyzed other conjunction (that is flattern AND).
…`betweenAnd` expression class and included some work related to unique functions. This time, the pick only included the changes related to `betweenAnd`.
What problem does this PR solve?
in logical plan builder, there are bugs with between:
a between random() and random(), since the two unbound'random()are the same, it will rewrite toa = random(), but the two random() should be different after bind expression;random() between 0.1 and 0.5, it will rewrite torandom() >= 0.1 and random() <= 0.5, later when bind expression, the two unbound random() will generate two different bounded random() function, but the two random() need to be the same.so, in logical plan builder, the between shouldn't compare low bound and upper bound, and should not expand before bind expression.
relate PR:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)