From 171c4dd9bc49b39ac2fa85dd64404a3791f2831c Mon Sep 17 00:00:00 2001 From: starocean999 <12095047@qq.com> Date: Thu, 21 Dec 2023 12:46:41 +0800 Subject: [PATCH 1/2] [fix](nereids)group by expr may be bound twice in bind agg slot --- .../rules/analysis/BindExpression.java | 32 +++++++++++++++---- .../nereids_syntax_p0/analyze_agg.groovy | 11 +++++++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java index 0437f7cdcdac3f..59441bd784b232 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java @@ -292,8 +292,19 @@ protected boolean condition(Rule rule, Plan plan) { if (alias.child().anyMatch(expr -> expr instanceof AggregateFunction)) { continue; } - // NOTICE: must use unbound expressions, because we will bind them in binding group by expr. - childOutputsToExpr.putIfAbsent(alias.getName(), agg.getOutputExpressions().get(i).child(0)); + /* + Alias(x) has been bound by binding agg's output + we add x to childOutputsToExpr, so when binding group by exprs later, we can use x directly + and won't bind it again + select + p_cycle_time / (select max(p_cycle_time) from log_event_8) as 'x', + count(distinct case_id) as 'y' + from + log_event_8 + group by + x + */ + childOutputsToExpr.putIfAbsent(alias.getName(), output.get(i).child(0)); } List replacedGroupBy = agg.getGroupByExpressions().stream() @@ -336,16 +347,23 @@ protected boolean condition(Rule rule, Plan plan) { List groupBy = replacedGroupBy.stream() .map(expression -> { - Expression e = binder.bind(expression); - if (e instanceof UnboundSlot) { - return childBinder.bind(e); + if (expression.hasUnbound()) { + // bind slot for unbound exprs + Expression e = binder.bind(expression); + if (e instanceof UnboundSlot) { + return childBinder.bind(e); + } + return e; + } else { + // expr has been bound by binding agg's output + return expression; } - return e; }) .collect(Collectors.toList()); groupBy.forEach(expression -> checkBoundExceptLambda(expression, ctx.root)); groupBy = groupBy.stream() - .map(expr -> bindFunction(expr, ctx.root, ctx.cascadesContext)) + // bind function for unbound exprs or return old expr if it's bound by binding agg's output + .map(expr -> expr.hasUnbound() ? bindFunction(expr, ctx.root, ctx.cascadesContext) : expr) .collect(ImmutableList.toImmutableList()); checkIfOutputAliasNameDuplicatedForGroupBy(groupBy, output); return agg.withGroupByAndOutput(groupBy, output); diff --git a/regression-test/suites/nereids_syntax_p0/analyze_agg.groovy b/regression-test/suites/nereids_syntax_p0/analyze_agg.groovy index 9f426e23e3d133..9a79df6bad524b 100644 --- a/regression-test/suites/nereids_syntax_p0/analyze_agg.groovy +++ b/regression-test/suites/nereids_syntax_p0/analyze_agg.groovy @@ -77,4 +77,15 @@ suite("analyze_agg") { // should not bind g /g in group by again, otherwise will throw exception sql "select g / g as nu, sum(c) from t2 group by nu" + sql """ + select + 1, + id / (select max(id) from t2) as 'x', + count(distinct c) as 'y' + from + t2 + group by + 1, + x + """ } \ No newline at end of file From a2f334268a94251941b1422b3cdac924e82c08c1 Mon Sep 17 00:00:00 2001 From: starocean999 <12095047@qq.com> Date: Thu, 21 Dec 2023 15:26:23 +0800 Subject: [PATCH 2/2] fix failed cast --- .../nereids/rules/analysis/BindExpression.java | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java index 59441bd784b232..83b0eb1ea7924e 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java @@ -307,6 +307,7 @@ protected boolean condition(Rule rule, Plan plan) { childOutputsToExpr.putIfAbsent(alias.getName(), output.get(i).child(0)); } + Set boundedGroupByExpressions = Sets.newHashSet(); List replacedGroupBy = agg.getGroupByExpressions().stream() .map(groupBy -> { if (groupBy instanceof UnboundSlot) { @@ -314,7 +315,9 @@ protected boolean condition(Rule rule, Plan plan) { if (unboundSlot.getNameParts().size() == 1) { String name = unboundSlot.getNameParts().get(0); if (childOutputsToExpr.containsKey(name)) { - return childOutputsToExpr.get(name); + Expression expression = childOutputsToExpr.get(name); + boundedGroupByExpressions.add(expression); + return expression; } } } @@ -347,23 +350,24 @@ protected boolean condition(Rule rule, Plan plan) { List groupBy = replacedGroupBy.stream() .map(expression -> { - if (expression.hasUnbound()) { + if (boundedGroupByExpressions.contains(expression)) { + // expr has been bound by binding agg's output + return expression; + } else { // bind slot for unbound exprs Expression e = binder.bind(expression); if (e instanceof UnboundSlot) { return childBinder.bind(e); } return e; - } else { - // expr has been bound by binding agg's output - return expression; } }) .collect(Collectors.toList()); groupBy.forEach(expression -> checkBoundExceptLambda(expression, ctx.root)); groupBy = groupBy.stream() // bind function for unbound exprs or return old expr if it's bound by binding agg's output - .map(expr -> expr.hasUnbound() ? bindFunction(expr, ctx.root, ctx.cascadesContext) : expr) + .map(expr -> boundedGroupByExpressions.contains(expr) ? expr + : bindFunction(expr, ctx.root, ctx.cascadesContext)) .collect(ImmutableList.toImmutableList()); checkIfOutputAliasNameDuplicatedForGroupBy(groupBy, output); return agg.withGroupByAndOutput(groupBy, output);