-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[enhance](nereids) add rule MultiDistinctSplit #45209
base: master
Are you sure you want to change the base?
[enhance](nereids) add rule MultiDistinctSplit #45209
Conversation
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
run buildall |
TPC-H: Total hot run time: 40494 ms
|
TPC-DS: Total hot run time: 196435 ms
|
ClickBench: Total hot run time: 32.27 s
|
fb681c9
to
99b8ad5
Compare
run buildall |
99b8ad5
to
bcd9fcc
Compare
run buildall |
TPC-H: Total hot run time: 39674 ms
|
TPC-DS: Total hot run time: 197910 ms
|
ClickBench: Total hot run time: 32.7 s
|
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAnalysis.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Rewriter.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctSplit.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctSplit.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctSplit.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctSplit.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctSplit.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctSplit.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctSplit.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctSplit.java
Outdated
Show resolved
Hide resolved
run buildall |
1 similar comment
run buildall |
d420066
to
93ee342
Compare
run buildall |
TPC-H: Total hot run time: 39791 ms
|
TPC-DS: Total hot run time: 196228 ms
|
ClickBench: Total hot run time: 32.23 s
|
5aa17ad
to
fbdbae1
Compare
run buildall |
TPC-H: Total hot run time: 40034 ms
|
TPC-DS: Total hot run time: 189796 ms
|
ClickBench: Total hot run time: 32.85 s
|
e798aac
to
a6f812c
Compare
run buildall |
422bb3a
to
2e6cb38
Compare
run p0 |
run buildall |
TPC-H: Total hot run time: 40224 ms
|
TPC-DS: Total hot run time: 189947 ms
|
ClickBench: Total hot run time: 31.79 s
|
@@ -1810,8 +1809,7 @@ private List<Expression> getHashAggregatePartitionExpressions( | |||
|
|||
private AggregateFunction tryConvertToMultiDistinct(AggregateFunction function) { | |||
if (function instanceof Count && function.isDistinct()) { |
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.
if (function instanceOf SupportMultiDistinct && function.isDistinct()) {
return function.convertToMultiDistinct();
}
|
||
package org.apache.doris.nereids.trees.expressions.functions.agg; | ||
|
||
/** MultiDistinctTrait*/ |
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.
could u add more detail comment to explain what is multi distinct aggregate function?
if (!distinctMultiColumns && !agg.getGroupByExpressions().isEmpty()) { | ||
return false; | ||
} |
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 comment to explain this if statement
private static boolean needTransform(LogicalAggregate<Plan> agg, List<Alias> aliases, List<Alias> otherAggFuncs) { | ||
// TODO with source repeat aggregate need to be supported in future | ||
if (agg.getSourceRepeat().isPresent()) { | ||
return false; |
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.
should check multiple count distinct with multi parameters and throw exception here?
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.
complete the description of this pr and the performance of test case
@@ -444,6 +445,7 @@ public class Rewriter extends AbstractBatchJobExecutor { | |||
new CollectCteConsumerOutput() | |||
) | |||
), | |||
// topic("distinct split", topDown(new DistinctSplit())), |
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.
remove this line
@@ -577,7 +577,7 @@ suite("partition_mv_rewrite_dimension_2_4") { | |||
count(distinct case when O_SHIPPRIORITY > 2 and o_orderkey IN (2) then o_custkey else null end) as cnt_2 | |||
from orders_2_4 | |||
where o_orderkey > (-3) + 5 """ | |||
mv_rewrite_success(sql_stmt_13, mv_name_13) | |||
mv_rewrite_fail(sql_stmt_13, mv_name_13) |
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.
this case meet rollback, or expect fail?
String sql = "select count(distinct b), count(distinct a) from test_distinct_multi"; | ||
PlanChecker.from(connectContext).checkExplain(sql, planner -> { | ||
Plan plan = planner.getOptimizedPlan(); | ||
MatchingUtils.assertMatches(plan, physicalCTEAnchor(physicalCTEProducer(any()), physicalResultSink(physicalProject(physicalNestedLoopJoin( |
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.
Use line break and indentation to format code to see the shape of plan, this lines width than 120 characters?
* +--LogicalAggregate(output:count(distinct b)) | ||
* +--LogicalCTEConsumer | ||
* */ | ||
public class DistinctSplit extends DefaultPlanRewriter<DistinctSplitContext> implements CustomRewriter { |
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.
SplitMultiDistinct
is more readable
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Rewriter.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctSplit.java
Outdated
Show resolved
Hide resolved
791a242
to
92ebe5c
Compare
run buildall |
92ebe5c
to
6603b90
Compare
add rule count distinct split add rule count distinct split add regression test add regression fix code style change by comment change to custom rewrite change to custom rewrite fix regression fix regression
6603b90
to
a6d2c1c
Compare
run buildall |
TPC-H: Total hot run time: 32459 ms
|
TPC-DS: Total hot run time: 189957 ms
|
ClickBench: Total hot run time: 31.28 s
|
run p0 |
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?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
This pr add a rewrite rule, which can do this 2 type of rewrite:
2.Before this PR, the following SQL statement would fail to execute due to an error: "The query contains multi count distinct or sum distinct, each can't have multi columns". This PR rewrites this type of SQL statement as follows, making it executable without an error.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)