-
Notifications
You must be signed in to change notification settings - Fork 3.7k
branch-3.0: [fix](nereids) do eliminate constant group by key in normalizeagg #49589 #50215
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
branch-3.0: [fix](nereids) do eliminate constant group by key in normalizeagg #49589 #50215
Conversation
…ache#49589) ### What problem does this PR solve? Related PR: apache#32878 apache#49473 Problem Summary: SELECT IF( t.`gender` IN ('女'), ( TIMESTAMPDIFF( YEAR, NOW(), NOW() ) ), 1 ) AS x0, TIMESTAMPDIFF( YEAR, NOW(), NOW() ) AS x1 FROM t1 AS t GROUP BY x0, x1; after EliminateGroupByConstant, this sql will be rewritten to SELECT IF( t.`gender` IN ('女'), 0, 1 ) AS x0, 0 AS x1 FROM t1 AS t GROUP BY IF( t.`gender` IN ('女'), ( TIMESTAMPDIFF( YEAR, NOW(), NOW() ) ), 1 ) ; The select expression and the group by expression is different, and will report error in normalizeagg. The fix in PR apache#49473 may introduce another issue. Consider the following query: SELECT func2(100) FROM t GROUP BY func1(), func2(func1()); If func1() can be constant-folded to 100, then func2(func1()) will be replaced with func2(100), allowing the query to execute successfully. However, when func1() cannot be folded to 100, the query will fail. This creates an inconsistent behavior where query execution depends on whether func1() can be constant-folded or not, which is not an ideal implementation. To address this issue, this PR modifies the normalizeAgg logic to eliminate constant group by keys. With this change, the query will consistently fail regardless of whether func1() can be folded or not, ensuring more predictable behavior.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
1 similar comment
|
run buildall |
TPC-H: Total hot run time: 39566 ms |
TPC-DS: Total hot run time: 196553 ms |
ClickBench: Total hot run time: 32.32 s |
dataroaring
left a comment
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.
LGTM
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
a865e24
|
run buildall |
TPC-H: Total hot run time: 40430 ms |
TPC-DS: Total hot run time: 196589 ms |
ClickBench: Total hot run time: 32.32 s |
dataroaring
left a comment
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.
LGTM
Cherry-picked from #49589