-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Planner support push down predicates past agg, win and sort #1471
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
morningman
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.
Please create an ISSUE to describe the problem and how you solve this problem.
| getIds(exprTupleIds, exprSlotIds); | ||
|
|
||
| final List<SlotId> intersection = Lists.newArrayList(); | ||
| intersection.addAll(exprSlotIds); |
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.
Why not just reuse the exprSlotIds to calculate the intersection?
like:
exprSlotIds.retainAll(slotIds);
And you can just !return exprSlotIds.retainAll(slotIds).
If exprSlotIds changes, retainAll will return true.
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.
because it change the param arg.
| * Push down predicates rules | ||
| */ | ||
|
|
||
| private void pushDownPredicates(Analyzer analyzer, SelectStmt stmt) throws AnalysisException { |
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 add some comment to these functions?
There is no comment at all?
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.
ok, i will explain it.
| return false; | ||
| } | ||
|
|
||
| private boolean putPredicatesOnFrom(SelectStmt stmt, Analyzer analyzer, List<Expr> predicates) throws AnalysisException { |
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.
Maybe the return of this function is 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.
Ok, i will remove it.
|
LGTM |
| for (SlotId slotId : slotIds) { | ||
| final SlotDescriptor slotDesc = analyzer.getDescTbl().getSlotDesc(slotId); | ||
| if (slotDesc.getSourceExprs().get(0).getFn() instanceof AggregateFunction) { | ||
| isAllSlotReferingGroupBys = 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.
break
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.
OK
| putPredicatesOnFrom(stmt, analyzer, pushDownPredicates); | ||
| } | ||
|
|
||
| private List<Expr> getPredicatesBoundedByGroupbysAndReplaceSlotWithSourceExpr(List<Expr> predicates, Analyzer analyzer) { |
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.
can you make this function name short
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.
OK, i will fix it.
morningman
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
#1438