-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[opt](nereids) support pushing down aggr distinct through join #43380
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. Since 2024-03-18, the Document has been moved to doris-website. |
7a772d1 to
29c1de5
Compare
|
run buildall |
29c1de5 to
964514c
Compare
|
run buildall |
| .when(agg -> agg.child().child().getOtherJoinConjuncts().isEmpty()) | ||
| .when(agg -> !agg.isGenerated()) | ||
| .whenNot(agg -> agg.getAggregateFunctions().isEmpty()) | ||
| .whenNot(agg -> agg.child().children().stream().anyMatch(p -> p instanceof LogicalAggregate)) |
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.
agg -> agg.child().children().stream().anyMatch(p -> p instanceof LogicalAggregate) , this is always 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.
thx, should be changed to .whenNot(agg -> agg.child()
.child(0).children().stream().anyMatch(p -> p instanceof LogicalAggregate))
| leftFuncs.forEach(func -> { | ||
| Alias alias = func.alias("PDADT_" + func.getName()); | ||
| leftSlotToOutput.put((Slot) func.child(0), alias); | ||
| }); |
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.
map leftSlotToOutput value seems to be useless
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
964514c to
e99aa8c
Compare
|
run buildall |
|
PR approved by anyone and no changes requested. |
e99aa8c to
8076eb0
Compare
|
run buildall |
|
run cloud_p0 |
1 similar comment
|
run cloud_p0 |
| public List<Rule> buildRules() { | ||
| return ImmutableList.of( | ||
| logicalAggregate(logicalProject(innerLogicalJoin())) | ||
| .when(agg -> agg.child().isAllSlots()) |
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.
agg.child().isAllSlots()
is this restriction too strong?
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.
aggr with distinct pushing down has special rewrite logic and restrict this case carefully by safety
| return ImmutableList.of( | ||
| logicalAggregate(logicalProject(innerLogicalJoin())) | ||
| .when(agg -> agg.child().isAllSlots()) | ||
| .when(agg -> agg.child().child().getOtherJoinConjuncts().isEmpty()) |
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.
"getOtherJoinConjuncts().isEmpty()"
how about remove this?
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.
leave this clean up task for the whole series of aggr pushing down rules
|
why it is called oneSide? it could push down through both sides |
|
run cloud_p0 |
8076eb0 to
b243038
Compare
|
run buildall |
|
run cloud_p0 |
bf3e09d to
5e048db
Compare
|
run buildall |
c85ccfa to
9e4d53c
Compare
|
run buildall |
|
run p0 |
1 similar comment
|
run p0 |
|
PR approved by at least one committer and no changes requested. |
What problem does this PR solve?
support pushing down agg distinct through join on one side.
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Release note
None
Check List (For Reviewer who merge this PR)