-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[feature](nereids) Convert subqueries into algebraic expressions and … #11454
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
| return logicalPlanToSubquery.get(plan); | ||
| } | ||
|
|
||
| public void setLogicalPlanToSubquery(LogicalPlan plan, SubqueryExpr subqueryExpr) { |
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.
| public void setLogicalPlanToSubquery(LogicalPlan plan, SubqueryExpr subqueryExpr) { | |
| public void setLogicalPlanToSubqueryIfAbsent(LogicalPlan plan, SubqueryExpr subqueryExpr) { |
| filter.child(), ctx.plannerContext)); | ||
| }) | ||
| ), | ||
| RuleType.ANALYZE_AGGREGATE_SUBQUERY.build( |
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.
sort and aggregate should not has subqueryExpr? And currently doris not support select subquery.
So you don't need to process LogicalProject, LogicalAggregate and LogicalSort.
| ); | ||
| } | ||
|
|
||
| private List<SubqueryExpr> extractSubquery(Expression expression) { |
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.
don't need to add extractSubquery function and getAllSubquery function, the shortly function is TreeNode::collect
| private List<SubqueryExpr> extractSubquery(Expression expression) { | |
| expression.collect(SubqueryExpr.class::isInstance) |
| private LogicalPlan addScalarSubqueryCorrelatedJoins(ScalarSubquery scalarSubquery, | ||
| LogicalPlan childPlan, PlannerContext ctx) { | ||
| LogicalPlan enforce = new LogicalEnforceSingleRow<>(scalarSubquery.getQueryPlan()); | ||
| scalarSubquery.setAnalyzed(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.
Immutable expression should not has mutable field. if we need some mutable state, we should depend the expression type to compute state, and change it when replace children
e.g.
UnboundExression.isAnalyzed() = false.
other expression.isAnalyzed() = Suppliers.memoized(() -> children().allMatch(Expression::isAnalyzed)).
| ctx.getSubquery(exists.getQueryPlan()).getCorrelateSlots()); | ||
| } | ||
|
|
||
| private LogicalPlan appendApplyNode(SubqueryExpr subqueryExpr, LogicalPlan childPlan, |
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 function not necessary, because the body is only one line and method parameter num is same, and this function is not about 'append'
| public Slot visitUnboundSlot(UnboundSlot unboundSlot, Void context) { | ||
| public Slot visitUnboundSlot(UnboundSlot unboundSlot, PlannerContext context) { | ||
| List<Slot> tmpBound = bindSlot(unboundSlot, getScope().getSlots()); | ||
| boolean hasCorrelate = 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.
this position should has a variable named as boolean foundInThisScope = !tmpBound.isEmpty();
and if foundInThisScope is false, then find in outerScope, if found in outerScope, then set boolean foundInOuterScope = true.
and then is the switch statement, you can declare boolean hasCorrelate = foundInOuterScope or use foundInOuterScope directly
| if (tmpBound.size() == 0) { | ||
| hasCorrelate = true; | ||
| } | ||
| Optional<List<Slot>> boundedOpt = getScope() |
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 compute bind slot in thisScope again, I think it should be
if (!foundInThisScope && getScope.getOuterScope().isPresent()) {
Optional<List<Slot>> boundedOpt = getScope()
.getOuterScope()
.get()
.toScopeLink()
.stream()
...
}| return checkSlots.contains(slot); | ||
| } | ||
|
|
||
| public void addSlot(Slot slot) { |
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.
checkSlots is final, so can not addSlot
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.
and checkSlots can rename to slotSet
|
Please rebase master to re-run P0 test |
|
I suggest first supporting analyzing and unnesting one type of subquery, e.g., converting |
47be108 to
89d0bc5
Compare
9347fc2 to
315945b
Compare
| /** | ||
| * Use the visitor to iterate sub expression. | ||
| */ | ||
| private static class SubExprAnalyzer<C> extends DefaultExpressionRewriter<C> { |
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.
| private static class SubExprAnalyzer<C> extends DefaultExpressionRewriter<C> { | |
| private static class SubExprAnalyzer extends DefaultExpressionRewriter<PlannerContext> { |
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
| assertNumRowsElement.getSubqueryString(), translateAsserTion(assertNumRowsElement.getAssertion())); | ||
| } | ||
|
|
||
| private static org.apache.doris.analysis.AssertNumRowsElement.Assertion translateAsserTion( |
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.
| private static org.apache.doris.analysis.AssertNumRowsElement.Assertion translateAsserTion( | |
| private static org.apache.doris.analysis.AssertNumRowsElement.Assertion translateAssertion( |
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
| case GE: | ||
| return Assertion.GE; | ||
| default: | ||
| return null; |
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.
return null is safe? should we throw a unsupported type 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.
done
| } | ||
|
|
||
| /** | ||
| * Convert expressions with subqueries in filter. |
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.
| * Convert expressions with subqueries in filter. | |
| * The Subquery in the LogicalFilter will change to LogicalApply, so we must replace the origin Subquery | |
| * in the LogicalFilter(the meaning is remove origin Subquery in the LogicalFilter). | |
| * | |
| * The replace rules are: |
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
| return replaceSubquery((SubqueryExpr) oldPredicate); | ||
| } | ||
| return newChildren.isEmpty() ? Optional.of(oldPredicate) : Optional.of(oldPredicate.withChildren( | ||
| newChildren.stream().map(expr -> expr.get()).collect(Collectors.toList()))); |
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(expr -> expr.get())
if you not check the Optional whether is present, why not remove Optional? Or you missing filter the present Optional 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.
done
| public class ApplyPullFilterOnProjectUnderAgg extends OneRewriteRuleFactory { | ||
| @Override | ||
| public Rule build() { | ||
| return logicalApply(any(), logicalAggregate(logicalProject(logicalFilter()))) |
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.
change any() to group()
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
| LogicalAggregate<LogicalProject<LogicalFilter<GroupPlan>>> agg = apply.right(); | ||
| if (!agg.getGroupByExpressions().isEmpty()) { | ||
| return apply; | ||
| } |
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.
you can move this condition to pattern.
logicalApply(
group(),
logicalAggregate(
logicalProject(logicalFilter()))
).when(agg -> agg.getGroupByExpressions().isEmpty())
).when(LogicalApply::isCorrelated).then(apply -> {})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.
deleted
| * | | ||
| * Filter(correlated predicate(Input.e = this.f)/Unapply predicate) | ||
| * | | ||
| * Project(output:a,child.output) |
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.
ditto, this project maybe add some expression that origin filter need, you should note it. And it is not 'child.output'
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
| public class PushApplyUnderProject extends OneRewriteRuleFactory { | ||
| @Override | ||
| public Rule build() { | ||
| return logicalApply(any(), logicalProject()).when(LogicalApply::isCorrelated).then(apply -> { |
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.
change any() to group()
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
| * input queryPlan input queryPlan | ||
| * | ||
| * UnCorrelated -> CROSS_JOIN(Count(1)) | ||
| * apply Filter(count(1) = 0) |
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.
missing CROSS_JOIN?
and Count(1) should change to Count(*)?
and we should add a limit(1) for performance?
filter(equal(cnt, 0))
|
aggregate(count(*) as cnt)
|
limit(1)
|
queryPlancount(1) not equal to count(*), maybe wrong.
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.
Forgot to modify the comments here, the code uses count(*), you can add a limit
…put them into the query tree
Proposed changes
Issue Number:
Problem summary
1.Convert subqueries to Apply nodes.
2.Convert ApplyNode to ordinary join.
Detailed design:
There are three types of current subexpressions, scalarSubquery, inSubquery, and Exists. The scalarSubquery refers to the returned data as 1 row and 1 column.
Subquery replacement
Subquery Transformation Rules
Checklist(Required)
Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...