morrySnow commented on code in PR #23271:
URL: https://github.com/apache/doris/pull/23271#discussion_r1305106926
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Analyzer.java:
##########
@@ -106,8 +108,14 @@ private static List<RewriteJob>
buildAnalyzeJobs(Optional<CustomTableResolver> c
// LogicalProject for normalize. This rule depends on
FillUpMissingSlots to fill up slots.
new NormalizeRepeat()
),
- bottomUp(new SubqueryToApply()),
bottomUp(new AdjustAggregateNullableForEmptySet()),
+ // run CheckAnalysis before EliminateGroupByConstant in order to
report error message correctly like bellow
+ // select SUM(lo_tax) FROM lineorder group by 1;
+ // errCode = 2, detailMessage = GROUP BY expression must not
contain aggregate functions: sum(lo_tax)
+ bottomUp(new CheckAnalysis()),
+ topDown(new EliminateGroupByConstant()),
+ topDown(new NormalizeAggregate()),
+ bottomUp(new SubqueryToApply()),
bottomUp(new CheckAnalysis())
Review Comment:
do we still need check again?
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/NormalizeAggregateTest.java:
##########
Review Comment:
move this file to rules/analyis
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SubqueryToApply.java:
##########
@@ -116,36 +113,45 @@ public List<Rule> buildRules() {
return new LogicalFilter<>(conjuncts, applyPlan);
})
),
- RuleType.PROJECT_SUBQUERY_TO_APPLY.build(
- logicalProject().thenApply(ctx -> {
- LogicalProject<Plan> project = ctx.root;
- Set<SubqueryExpr> subqueryExprs = new LinkedHashSet<>();
- project.getProjects().stream()
- .filter(Alias.class::isInstance)
- .map(Alias.class::cast)
- .filter(alias -> alias.child() instanceof CaseWhen)
- .forEach(alias -> alias.child().children().stream()
- .forEach(e ->
-
subqueryExprs.addAll(e.collect(SubqueryExpr.class::isInstance))));
- if (subqueryExprs.isEmpty()) {
- return project;
- }
-
- SubqueryContext context = new
SubqueryContext(subqueryExprs);
- return new LogicalProject(project.getProjects().stream()
- .map(p -> p.withChildren(
- new ReplaceSubquery(ctx.statementContext, true)
- .replace(p, context)))
- .collect(ImmutableList.toImmutableList()),
- subqueryToApply(
-
subqueryExprs.stream().collect(ImmutableList.toImmutableList()),
- (LogicalPlan) project.child(),
- context.getSubqueryToMarkJoinSlot(),
- ctx.cascadesContext,
- Optional.empty(), true
- ));
- })
- )
+
RuleType.PROJECT_SUBQUERY_TO_APPLY.build(logicalProject().thenApply(ctx -> {
+ LogicalProject<Plan> project = ctx.root;
+ ImmutableList<Set> subqueryExprsList =
project.getProjects().stream()
+ .map(e -> (Set)
e.collect(SubqueryExpr.class::isInstance))
+ .collect(ImmutableList.toImmutableList());
+ if (subqueryExprsList.stream().flatMap(Collection::stream)
+ .noneMatch(SubqueryExpr.class::isInstance)) {
Review Comment:
```suggestion
if
(subqueryExprsList.stream().flatMap(Collection::stream).count() == 0) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SubqueryToApply.java:
##########
@@ -116,36 +113,45 @@ public List<Rule> buildRules() {
return new LogicalFilter<>(conjuncts, applyPlan);
})
),
- RuleType.PROJECT_SUBQUERY_TO_APPLY.build(
- logicalProject().thenApply(ctx -> {
- LogicalProject<Plan> project = ctx.root;
- Set<SubqueryExpr> subqueryExprs = new LinkedHashSet<>();
- project.getProjects().stream()
- .filter(Alias.class::isInstance)
- .map(Alias.class::cast)
- .filter(alias -> alias.child() instanceof CaseWhen)
- .forEach(alias -> alias.child().children().stream()
- .forEach(e ->
-
subqueryExprs.addAll(e.collect(SubqueryExpr.class::isInstance))));
- if (subqueryExprs.isEmpty()) {
- return project;
- }
-
- SubqueryContext context = new
SubqueryContext(subqueryExprs);
- return new LogicalProject(project.getProjects().stream()
- .map(p -> p.withChildren(
- new ReplaceSubquery(ctx.statementContext, true)
- .replace(p, context)))
- .collect(ImmutableList.toImmutableList()),
- subqueryToApply(
-
subqueryExprs.stream().collect(ImmutableList.toImmutableList()),
- (LogicalPlan) project.child(),
- context.getSubqueryToMarkJoinSlot(),
- ctx.cascadesContext,
- Optional.empty(), true
- ));
- })
- )
+
RuleType.PROJECT_SUBQUERY_TO_APPLY.build(logicalProject().thenApply(ctx -> {
+ LogicalProject<Plan> project = ctx.root;
+ ImmutableList<Set> subqueryExprsList =
project.getProjects().stream()
+ .map(e -> (Set)
e.collect(SubqueryExpr.class::isInstance))
+ .collect(ImmutableList.toImmutableList());
+ if (subqueryExprsList.stream().flatMap(Collection::stream)
+ .noneMatch(SubqueryExpr.class::isInstance)) {
+ return project;
+ }
+ List<NamedExpression> oldProjects =
ImmutableList.copyOf(project.getProjects());
+ List<NamedExpression> newProjects = Lists.newArrayList();
Review Comment:
use `ImmutableList.Builder` instead of `Lists.newArrayList()`
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushdownAliasThroughJoin.java:
##########
@@ -45,7 +46,9 @@ public class PushdownAliasThroughJoin extends
OneRewriteRuleFactory {
public Rule build() {
return logicalProject(logicalJoin())
.when(project -> project.getProjects().stream().allMatch(expr ->
- (expr instanceof Slot) || (expr instanceof Alias &&
((Alias) expr).child() instanceof Slot)))
+ (expr instanceof Slot && !(expr instanceof
MarkJoinSlotReference))
+ || (expr instanceof Alias && ((Alias) expr).child()
instanceof Slot
+ && !(((Alias) expr).child() instanceof
MarkJoinSlotReference))))
Review Comment:
do we need add more ut about this change?
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/ExtractAndNormalizeWindowExpressionTest.java:
##########
Review Comment:
move this file to rules/analyis
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Rewriter.java:
##########
@@ -138,6 +139,10 @@ public class Rewriter extends AbstractBatchJobExecutor {
),
// subquery unnesting relay on ExpressionNormalization to extract
common factor expression
topic("Subquery unnesting",
+ // after doing NormalizeAggregate in analysis job
+ // we need run the following 2 rules to make
AGG_SCALAR_SUBQUERY_TO_WINDOW_FUNCTION work
+ bottomUp(new PullUpProjectUnderApply()),
+ topDown(new PushdownFilterThroughProject()),
Review Comment:
could we put them into costBased(...) ?
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/EliminateGroupByConstantTest.java:
##########
Review Comment:
move this file to rules/analyis
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SubqueryToApply.java:
##########
@@ -116,36 +113,45 @@ public List<Rule> buildRules() {
return new LogicalFilter<>(conjuncts, applyPlan);
})
),
- RuleType.PROJECT_SUBQUERY_TO_APPLY.build(
- logicalProject().thenApply(ctx -> {
- LogicalProject<Plan> project = ctx.root;
- Set<SubqueryExpr> subqueryExprs = new LinkedHashSet<>();
- project.getProjects().stream()
- .filter(Alias.class::isInstance)
- .map(Alias.class::cast)
- .filter(alias -> alias.child() instanceof CaseWhen)
- .forEach(alias -> alias.child().children().stream()
- .forEach(e ->
-
subqueryExprs.addAll(e.collect(SubqueryExpr.class::isInstance))));
- if (subqueryExprs.isEmpty()) {
- return project;
- }
-
- SubqueryContext context = new
SubqueryContext(subqueryExprs);
- return new LogicalProject(project.getProjects().stream()
- .map(p -> p.withChildren(
- new ReplaceSubquery(ctx.statementContext, true)
- .replace(p, context)))
- .collect(ImmutableList.toImmutableList()),
- subqueryToApply(
-
subqueryExprs.stream().collect(ImmutableList.toImmutableList()),
- (LogicalPlan) project.child(),
- context.getSubqueryToMarkJoinSlot(),
- ctx.cascadesContext,
- Optional.empty(), true
- ));
- })
- )
+
RuleType.PROJECT_SUBQUERY_TO_APPLY.build(logicalProject().thenApply(ctx -> {
+ LogicalProject<Plan> project = ctx.root;
+ ImmutableList<Set> subqueryExprsList =
project.getProjects().stream()
+ .map(e -> (Set)
e.collect(SubqueryExpr.class::isInstance))
+ .collect(ImmutableList.toImmutableList());
Review Comment:
```suggestion
ImmutableList<Set<SubqueryExpr>> subqueryExprsList =
project.getProjects().stream()
.<Set<SubqueryExpr>>map(e ->
e.collect(SubqueryExpr.class::isInstance))
.collect(ImmutableList.toImmutableList());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
