Skip to content

Conversation

@chenhao7253886
Copy link
Contributor

@chenhao7253886 chenhao7253886 commented Jul 14, 2019

Copy link
Contributor

@morningman morningman left a 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.

@chenhao7253886 chenhao7253886 changed the title Planner support push down predicates past agg, win and sort Planner support push down predicates past agg, win and sort #1438 Jul 15, 2019
@chenhao7253886 chenhao7253886 changed the title Planner support push down predicates past agg, win and sort #1438 Planner support push down predicates past agg, win and sort Jul 15, 2019
getIds(exprTupleIds, exprSlotIds);

final List<SlotId> intersection = Lists.newArrayList();
intersection.addAll(exprSlotIds);
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@EmmyMiao87
Copy link
Contributor

LGTM

for (SlotId slotId : slotIds) {
final SlotDescriptor slotDesc = analyzer.getDescTbl().getSlotDesc(slotId);
if (slotDesc.getSourceExprs().get(0).getFn() instanceof AggregateFunction) {
isAllSlotReferingGroupBys = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break

Copy link
Contributor Author

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) {
Copy link
Contributor

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

Copy link
Contributor Author

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.

@imay imay closed this Sep 4, 2019
@imay imay reopened this Sep 4, 2019
Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@morningman morningman merged commit f23ac0e into apache:master Sep 8, 2019
luwei16 pushed a commit to luwei16/incubator-doris that referenced this pull request Apr 7, 2023
swjtu-zhanglei pushed a commit to swjtu-zhanglei/incubator-doris that referenced this pull request Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants