[feature](nereids) support pull up predicates from aggregate min/max/avg #42199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
|
run buildall |
TPC-H: Total hot run time: 41433 ms |
TPC-DS: Total hot run time: 191978 ms |
ClickBench: Total hot run time: 32.26 s |
feiniaofeiafei
force-pushed
the
add_pull_up_agg
branch
from
863e2ea to
6e9fc1a
Compare
|
run buildall |
feiniaofeiafei
force-pushed
the
add_pull_up_agg
branch
2 times, most recently
from
0798760 to
07b17d3
Compare
|
run buildall |
TPC-H: Total hot run time: 41601 ms |
TPC-DS: Total hot run time: 192714 ms |
ClickBench: Total hot run time: 33.29 s |
|
run feut |
xzj7019
reviewed
Oct 29, 2024
| @@ -325,8 +343,8 @@ private ImmutableSet<Expression> getAvailableExpressions(Set<Expression> predica | |||
| return ImmutableSet.copyOf(newPredicates); | |||
| } | |||
|
|
|||
| private boolean hasAgg(Expression expression) { | |||
| return expression.anyMatch(AggregateFunction.class::isInstance); | |||
| private boolean supportPullUpAgg(Expression expr) { | |||
There was a problem hiding this comment.
Does it mean the original hasAgg judgement actually contains any unexpected pulled up agg cases, after this restriction?
feiniaofeiafei
force-pushed
the
add_pull_up_agg
branch
from
07b17d3 to
f1e405f
Compare
|
run buildall |
|
run cloud_p0 |
|
run performance |
TPC-H: Total hot run time: 41452 ms |
TPC-DS: Total hot run time: 196276 ms |
ClickBench: Total hot run time: 33.03 s |
feiniaofeiafei
force-pushed
the
add_pull_up_agg
branch
from
f1e405f to
ecdd072
Compare
|
run buildall |
TPC-H: Total hot run time: 41598 ms |
TPC-DS: Total hot run time: 197942 ms |
ClickBench: Total hot run time: 33.25 s |
feiniaofeiafei
force-pushed
the
add_pull_up_agg
branch
from
ecdd072 to
7e6e02c
Compare
|
run buildall |
TPC-H: Total hot run time: 32592 ms |
TPC-DS: Total hot run time: 186664 ms |
ClickBench: Total hot run time: 30.29 s |
feiniaofeiafei
force-pushed
the
add_pull_up_agg
branch
from
7e6e02c to
9d7d133
Compare
…avg, and add qualifier when pull up predicates from project
feiniaofeiafei
force-pushed
the
add_pull_up_agg
branch
from
9d7d133 to
d9beba2
Compare
|
run buildall |
TPC-H: Total hot run time: 32422 ms |
TPC-DS: Total hot run time: 185869 ms |
ClickBench: Total hot run time: 30.77 s |
|
@xzj7019 PTAL |
morrySnow
reviewed
Feb 14, 2025
| @@ -379,4 +397,16 @@ private ImmutableSet<Expression> getFiltersFromUnionConstExprs(LogicalUnion unio | |||
| } | |||
| return ImmutableSet.copyOf(filtersFromConstExprs); | |||
| } | |||
|
|
|||
| private Map<Slot, Expression> generateMap(List<NamedExpression> namedExpressions) { | |||
There was a problem hiding this comment.
could u add some comment to explain the purpose of this replace map
Comment on lines
+406
to
+407
| } else if (namedExpression instanceof SlotReference) { | ||
| replaceMap.putIfAbsent((Slot) namedExpression, namedExpression); |
There was a problem hiding this comment.
why put a entry with same key and value? i think it is not necessary
Comment on lines
+288
to
+289
| Map<Expression, List<Slot>> expressionSlotMap | ||
| = Maps.newHashMapWithExpectedSize(outputExpressions.size()); |
There was a problem hiding this comment.
not need to keep order anymore?
Comment on lines
+291
to
+294
| if (output instanceof Alias && supportPullUpAgg(output.child(0))) { | ||
| expressionSlotMap.computeIfAbsent(output.child(0).child(0), | ||
| k -> new ArrayList<>()).add(output.toSlot()); | ||
| } |
There was a problem hiding this comment.
maybe need add some comment to explain when supportPullUpAgg return true, output.child(0) always has one child
| @@ -68,9 +77,11 @@ public class PullUpPredicates extends PlanVisitor<ImmutableSet<Expression>, Void | |||
|
|
|||
| Map<Plan, ImmutableSet<Expression>> cache = new IdentityHashMap<>(); | |||
| private final boolean getAllPredicates; | |||
| private final ExpressionRewriteContext rewriteContext; | |||
There was a problem hiding this comment.
could we add more detail about this rule in the class' comment or in each function's comment?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this pr do 2 things:
1.support pull up predicates from aggregate min/max/avg. e.g.
c<=10 pull up from aggregate to avg(c)<=10, and then pull up from aggregate to t.c1<=10, and with equal join condition,can infer predicate t2.a<=10.
2. carry qualifier when pull up from projects, before this pr, c1<=10 pull up from project to c1<=10. after this pr, c1<=10 pull up from project to t.c1<=10.