From c9d1fbcd7047d5efc1f8c1d955c4cf779fe7cd84 Mon Sep 17 00:00:00 2001 From: feiniaofeiafei Date: Wed, 20 Mar 2024 15:54:13 +0800 Subject: [PATCH 1/7] [Fix](nereids) modify the binding aggregate function in order by --- .../rules/analysis/BindExpression.java | 52 ++++++++++++++++++- 1 file changed, 50 insertions(+), 2 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 43f89d5b010c99..dc3a43c966469a 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 @@ -52,6 +52,7 @@ import org.apache.doris.nereids.trees.expressions.functions.FunctionBuilder; import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction; import org.apache.doris.nereids.trees.expressions.functions.generator.TableGeneratingFunction; +import org.apache.doris.nereids.trees.expressions.functions.scalar.Exp; import org.apache.doris.nereids.trees.expressions.functions.scalar.GroupingScalarFunction; import org.apache.doris.nereids.trees.expressions.functions.scalar.Lambda; import org.apache.doris.nereids.trees.expressions.functions.scalar.StructElement; @@ -759,7 +760,7 @@ private Plan bindSortWithoutSetOperation(MatchingContext> ctx) final Plan finalInput = input; Supplier inputChildrenScope = Suppliers.memoize( () -> toScope(cascadesContext, PlanUtils.fastGetChildrenOutputs(finalInput.children()))); - SimpleExprAnalyzer analyzer = buildCustomSlotBinderAnalyzer( + SimpleExprAnalyzer bindInInputScopeThenInputChildScope = buildCustomSlotBinderAnalyzer( sort, cascadesContext, inputScope, true, false, (self, unboundSlot) -> { // first, try to bind slot in Scope(input.output) @@ -774,9 +775,39 @@ private Plan bindSortWithoutSetOperation(MatchingContext> ctx) return self.bindExactSlotsByThisScope(unboundSlot, inputChildrenScope.get()); }); + ImmutableList.Builder outputSlots = ImmutableList.builder(); + if (finalInput instanceof LogicalAggregate) { + LogicalAggregate aggregate = (LogicalAggregate) finalInput; + List outputExpressions = aggregate.getOutputExpressions(); + for (NamedExpression outputExpr : outputExpressions) { + if (!outputExpr.anyMatch(expr -> expr instanceof AggregateFunction)) { + outputSlots.add(outputExpr.toSlot()); + } + } + } + Scope outputWithoutAggFunc = toScope(cascadesContext, outputSlots.build()); + SimpleExprAnalyzer bindInInputChildScope = buildCustomSlotBinderAnalyzer( + sort, cascadesContext, inputScope, true, false, + (analyzer, unboundSlot) -> { + if (finalInput instanceof LogicalAggregate) { + List boundInOutputWithoutAggFunc = analyzer.bindSlotByScope(unboundSlot, + outputWithoutAggFunc); + if (!boundInOutputWithoutAggFunc.isEmpty()) { + return ImmutableList.of(boundInOutputWithoutAggFunc.get(0)); + } + } + return analyzer.bindExactSlotsByThisScope(unboundSlot, inputChildrenScope.get()); + }); + Builder boundOrderKeys = ImmutableList.builderWithExpectedSize(sort.getOrderKeys().size()); + FunctionRegistry functionRegistry = cascadesContext.getConnectContext().getEnv().getFunctionRegistry(); for (OrderKey orderKey : sort.getOrderKeys()) { - Expression boundKey = bindWithOrdinal(orderKey.getExpr(), analyzer, childOutput); + Expression boundKey; + if (hasAggregateFunction(orderKey.getExpr(), functionRegistry)) { + boundKey = bindInInputChildScope.analyze(orderKey.getExpr()); + } else { + boundKey = bindWithOrdinal(orderKey.getExpr(), bindInInputScopeThenInputChildScope, childOutput); + } boundOrderKeys.add(orderKey.withExpression(boundKey)); } return new LogicalSort<>(boundOrderKeys.build(), sort.child()); @@ -965,4 +996,21 @@ default Set analyzeToSet(List exprs) { private interface CustomSlotBinderAnalyzer { List bindSlot(ExpressionAnalyzer analyzer, UnboundSlot unboundSlot); } + + private boolean hasAggregateFunction(Expression expression, FunctionRegistry functionRegistry) { + return expression.anyMatch(expr -> { + if (expr instanceof AggregateFunction) { + return true; + } else if (expr instanceof UnboundFunction) { + UnboundFunction unboundFunction = (UnboundFunction) expr; + boolean isAggregateFunction = functionRegistry + .isAggregateFunction( + unboundFunction.getDbName(), + unboundFunction.getName() + ); + return isAggregateFunction; + } + return false; + }); + } } From b939f98c7cf7389fc3f33ce091dfc7beca01b472 Mon Sep 17 00:00:00 2001 From: feiniaofeiafei Date: Sun, 24 Mar 2024 22:05:40 +0800 Subject: [PATCH 2/7] [Fix](nereids) modify the binding aggregate function in order by --- .../rules/analysis/BindExpression.java | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) 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 dc3a43c966469a..a479ea4ab66f0d 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 @@ -52,7 +52,6 @@ import org.apache.doris.nereids.trees.expressions.functions.FunctionBuilder; import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction; import org.apache.doris.nereids.trees.expressions.functions.generator.TableGeneratingFunction; -import org.apache.doris.nereids.trees.expressions.functions.scalar.Exp; import org.apache.doris.nereids.trees.expressions.functions.scalar.GroupingScalarFunction; import org.apache.doris.nereids.trees.expressions.functions.scalar.Lambda; import org.apache.doris.nereids.trees.expressions.functions.scalar.StructElement; @@ -899,6 +898,7 @@ private boolean isAggregateFunction(UnboundFunction unboundFunction, FunctionReg unboundFunction.getDbName(), unboundFunction.getName()); } +<<<<<<< HEAD private Expression bindWithOrdinal( Expression unbound, SimpleExprAnalyzer analyzer, List boundSelectOutput) { if (unbound instanceof IntegerLikeLiteral) { @@ -912,6 +912,23 @@ private Expression bindWithOrdinal( } else { return analyzer.analyze(unbound); } +======= + private boolean hasAggregateFunction(Expression expression, FunctionRegistry functionRegistry) { + return expression.anyMatch(expr -> { + if (expr instanceof AggregateFunction) { + return true; + } else if (expr instanceof UnboundFunction) { + UnboundFunction unboundFunction = (UnboundFunction) expr; + boolean isAggregateFunction = functionRegistry + .isAggregateFunction( + unboundFunction.getDbName(), + unboundFunction.getName() + ); + return isAggregateFunction; + } + return false; + }); +>>>>>>> a876efa500 ([Fix](nereids) modify the binding aggregate function in order by) } private E checkBoundExceptLambda(E expression, Plan plan) { From 5f15b008085e61a6566a83707284fe0f2cf81ee4 Mon Sep 17 00:00:00 2001 From: feiniaofeiafei Date: Mon, 25 Mar 2024 15:00:16 +0800 Subject: [PATCH 3/7] [Fix](nereids) modify the binding aggregate function in order by --- .../rules/analysis/FillUpMissingSlots.java | 2 +- .../order_by_bind_priority.groovy | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/FillUpMissingSlots.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/FillUpMissingSlots.java index 82468978a8069a..82de4453c131f1 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/FillUpMissingSlots.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/FillUpMissingSlots.java @@ -218,7 +218,7 @@ public void resolve(Expression expression) { // We couldn't find the equivalent expression in output expressions and group-by expressions, // so we should check whether the expression is valid. if (expression instanceof SlotReference) { - throw new AnalysisException(expression.toSql() + " in having clause should be grouped by."); + throw new AnalysisException(expression.toSql() + " should be grouped by."); } else if (expression instanceof AggregateFunction) { if (checkWhetherNestedAggregateFunctionsExist((AggregateFunction) expression)) { throw new AnalysisException("Aggregate functions in having clause can't be nested: " diff --git a/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy b/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy new file mode 100644 index 00000000000000..9cdb5f7845e452 --- /dev/null +++ b/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy @@ -0,0 +1,37 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("order_by_bind_priority") { + sql "SET enable_nereids_planner=true;" + sql "SET enable_fallback_to_original_planner=false;" + + sql "drop table if exists t_order_by_bind_priority" + sql """create table t_order_by_bind_priority (c1 int, c2 int) distributed by hash(c1) properties("replication_num"="1");""" + sql "insert into t_order_by_bind_priority values(-2, -2),(1,2),(1,2),(3,3),(-5,5);" + sql "sync" + + + sql "select 2*abs(sum(c1)) as c1, c1,sum(c1)+c1 from t_order_by_bind_priority group by c1 order by sum(c1)+c1 asc;" + sql "select 2*abs(sum(c1)) as c2, c1,sum(c1)+c2 from t_order_by_bind_priority group by c1,c2 order by sum(c1)+c2 asc;" + sql "select abs(sum(c1)) as c1, c1,sum(c2) as c2 from t_order_by_bind_priority group by c1 order by sum(c1) asc;" + test { + sql "select abs(sum(c1)) as c1, c1,sum(c2) as c2 from t_order_by_bind_priority group by c1 order by sum(c1)+c2 asc;" + exception "c2 should be grouped by." + } + + +} \ No newline at end of file From c26ea3a37a3626c45c243b050e4d4baee14bd8a8 Mon Sep 17 00:00:00 2001 From: feiniaofeiafei Date: Mon, 25 Mar 2024 15:07:24 +0800 Subject: [PATCH 4/7] [Fix](nereids) modify the binding aggregate function in order by --- .../suites/nereids_syntax_p0/order_by_bind_priority.groovy | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy b/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy index 9cdb5f7845e452..5546202bb99b11 100644 --- a/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy +++ b/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy @@ -25,9 +25,9 @@ suite("order_by_bind_priority") { sql "sync" - sql "select 2*abs(sum(c1)) as c1, c1,sum(c1)+c1 from t_order_by_bind_priority group by c1 order by sum(c1)+c1 asc;" - sql "select 2*abs(sum(c1)) as c2, c1,sum(c1)+c2 from t_order_by_bind_priority group by c1,c2 order by sum(c1)+c2 asc;" - sql "select abs(sum(c1)) as c1, c1,sum(c2) as c2 from t_order_by_bind_priority group by c1 order by sum(c1) asc;" + qt_test_bind_order_by_with_aggfun1 "select 2*abs(sum(c1)) as c1, c1,sum(c1)+c1 from t_order_by_bind_priority group by c1 order by sum(c1)+c1 asc;" + qt_test_bind_order_by_with_aggfun2 "select 2*abs(sum(c1)) as c2, c1,sum(c1)+c2 from t_order_by_bind_priority group by c1,c2 order by sum(c1)+c2 asc;" + qt_test_bind_order_by_with_aggfun3 "select abs(sum(c1)) as c1, c1,sum(c2) as c2 from t_order_by_bind_priority group by c1 order by sum(c1) asc;" test { sql "select abs(sum(c1)) as c1, c1,sum(c2) as c2 from t_order_by_bind_priority group by c1 order by sum(c1)+c2 asc;" exception "c2 should be grouped by." From f1c6b7639805b6c53040e48ee54dcc59e93afc66 Mon Sep 17 00:00:00 2001 From: feiniaofeiafei Date: Mon, 25 Mar 2024 16:18:54 +0800 Subject: [PATCH 5/7] [Fix](nereids) modify the binding aggregate function in order by --- .../order_by_bind_priority.out | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 regression-test/data/nereids_syntax_p0/order_by_bind_priority.out diff --git a/regression-test/data/nereids_syntax_p0/order_by_bind_priority.out b/regression-test/data/nereids_syntax_p0/order_by_bind_priority.out new file mode 100644 index 00000000000000..5c9a49fccc6815 --- /dev/null +++ b/regression-test/data/nereids_syntax_p0/order_by_bind_priority.out @@ -0,0 +1,19 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !test_bind_order_by_with_aggfun1 -- +10 -5 -10 +4 -2 -4 +4 1 3 +6 3 6 + +-- !test_bind_order_by_with_aggfun2 -- +4 -2 -4 +10 -5 0 +4 1 4 +6 3 6 + +-- !test_bind_order_by_with_aggfun3 -- +5 -5 5 +2 -2 -2 +2 1 4 +3 3 3 + From 4c2f7b210bf1932d019f056b94bbd597a12827c2 Mon Sep 17 00:00:00 2001 From: feiniaofeiafei Date: Tue, 26 Mar 2024 16:30:51 +0800 Subject: [PATCH 6/7] [Fix](nereids) modify the binding aggregate function in order by --- .../nereids/rules/analysis/BindExpression.java | 18 ------------------ .../rules/analysis/FillUpMissingSlotsTest.java | 2 +- .../order_by_bind_priority.out | 6 ++++++ .../order_by_bind_priority.groovy | 1 + 4 files changed, 8 insertions(+), 19 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 a479ea4ab66f0d..722b89c4c85746 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 @@ -898,7 +898,6 @@ private boolean isAggregateFunction(UnboundFunction unboundFunction, FunctionReg unboundFunction.getDbName(), unboundFunction.getName()); } -<<<<<<< HEAD private Expression bindWithOrdinal( Expression unbound, SimpleExprAnalyzer analyzer, List boundSelectOutput) { if (unbound instanceof IntegerLikeLiteral) { @@ -912,23 +911,6 @@ private Expression bindWithOrdinal( } else { return analyzer.analyze(unbound); } -======= - private boolean hasAggregateFunction(Expression expression, FunctionRegistry functionRegistry) { - return expression.anyMatch(expr -> { - if (expr instanceof AggregateFunction) { - return true; - } else if (expr instanceof UnboundFunction) { - UnboundFunction unboundFunction = (UnboundFunction) expr; - boolean isAggregateFunction = functionRegistry - .isAggregateFunction( - unboundFunction.getDbName(), - unboundFunction.getName() - ); - return isAggregateFunction; - } - return false; - }); ->>>>>>> a876efa500 ([Fix](nereids) modify the binding aggregate function in order by) } private E checkBoundExceptLambda(E expression, Plan plan) { diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/FillUpMissingSlotsTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/FillUpMissingSlotsTest.java index dbf65decef95f5..58ce64c09b2dcf 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/FillUpMissingSlotsTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/FillUpMissingSlotsTest.java @@ -306,7 +306,7 @@ void testJoinWithHaving() { void testInvalidHaving() { ExceptionChecker.expectThrowsWithMsg( AnalysisException.class, - "a2 in having clause should be grouped by.", + "a2 should be grouped by.", () -> PlanChecker.from(connectContext).analyze( "SELECT a1 FROM t1 GROUP BY a1 HAVING a2 > 0" )); diff --git a/regression-test/data/nereids_syntax_p0/order_by_bind_priority.out b/regression-test/data/nereids_syntax_p0/order_by_bind_priority.out index 5c9a49fccc6815..3c7dcb1d31844c 100644 --- a/regression-test/data/nereids_syntax_p0/order_by_bind_priority.out +++ b/regression-test/data/nereids_syntax_p0/order_by_bind_priority.out @@ -17,3 +17,9 @@ 2 1 4 3 3 3 +-- !test_bind_order_by_in_no_agg_func_output -- +1 4 +2 -2 +3 3 +5 5 + diff --git a/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy b/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy index 5546202bb99b11..e434ab42092948 100644 --- a/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy +++ b/regression-test/suites/nereids_syntax_p0/order_by_bind_priority.groovy @@ -28,6 +28,7 @@ suite("order_by_bind_priority") { qt_test_bind_order_by_with_aggfun1 "select 2*abs(sum(c1)) as c1, c1,sum(c1)+c1 from t_order_by_bind_priority group by c1 order by sum(c1)+c1 asc;" qt_test_bind_order_by_with_aggfun2 "select 2*abs(sum(c1)) as c2, c1,sum(c1)+c2 from t_order_by_bind_priority group by c1,c2 order by sum(c1)+c2 asc;" qt_test_bind_order_by_with_aggfun3 "select abs(sum(c1)) as c1, c1,sum(c2) as c2 from t_order_by_bind_priority group by c1 order by sum(c1) asc;" + qt_test_bind_order_by_in_no_agg_func_output "select abs(c1) xx, sum(c2) from t_order_by_bind_priority group by xx order by min(xx)" test { sql "select abs(sum(c1)) as c1, c1,sum(c2) as c2 from t_order_by_bind_priority group by c1 order by sum(c1)+c2 asc;" exception "c2 should be grouped by." From d232508b98153bc02df441a02569f65e022c2f4d Mon Sep 17 00:00:00 2001 From: feiniaofeiafei Date: Tue, 2 Apr 2024 16:40:22 +0800 Subject: [PATCH 7/7] [Fix](nereids) modify the binding aggregate function in order by --- .../rules/analysis/BindExpression.java | 54 ++++++++++--------- 1 file changed, 30 insertions(+), 24 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 722b89c4c85746..8c8a447e830ea8 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 @@ -774,30 +774,8 @@ private Plan bindSortWithoutSetOperation(MatchingContext> ctx) return self.bindExactSlotsByThisScope(unboundSlot, inputChildrenScope.get()); }); - ImmutableList.Builder outputSlots = ImmutableList.builder(); - if (finalInput instanceof LogicalAggregate) { - LogicalAggregate aggregate = (LogicalAggregate) finalInput; - List outputExpressions = aggregate.getOutputExpressions(); - for (NamedExpression outputExpr : outputExpressions) { - if (!outputExpr.anyMatch(expr -> expr instanceof AggregateFunction)) { - outputSlots.add(outputExpr.toSlot()); - } - } - } - Scope outputWithoutAggFunc = toScope(cascadesContext, outputSlots.build()); - SimpleExprAnalyzer bindInInputChildScope = buildCustomSlotBinderAnalyzer( - sort, cascadesContext, inputScope, true, false, - (analyzer, unboundSlot) -> { - if (finalInput instanceof LogicalAggregate) { - List boundInOutputWithoutAggFunc = analyzer.bindSlotByScope(unboundSlot, - outputWithoutAggFunc); - if (!boundInOutputWithoutAggFunc.isEmpty()) { - return ImmutableList.of(boundInOutputWithoutAggFunc.get(0)); - } - } - return analyzer.bindExactSlotsByThisScope(unboundSlot, inputChildrenScope.get()); - }); - + SimpleExprAnalyzer bindInInputChildScope = getAnalyzerForOrderByAggFunc(finalInput, cascadesContext, sort, + inputChildrenScope, inputScope); Builder boundOrderKeys = ImmutableList.builderWithExpectedSize(sort.getOrderKeys().size()); FunctionRegistry functionRegistry = cascadesContext.getConnectContext().getEnv().getFunctionRegistry(); for (OrderKey orderKey : sort.getOrderKeys()) { @@ -1012,4 +990,32 @@ private boolean hasAggregateFunction(Expression expression, FunctionRegistry fun return false; }); } + + private SimpleExprAnalyzer getAnalyzerForOrderByAggFunc(Plan finalInput, CascadesContext cascadesContext, + LogicalSort sort, Supplier inputChildrenScope, Scope inputScope) { + ImmutableList.Builder outputSlots = ImmutableList.builder(); + if (finalInput instanceof LogicalAggregate) { + LogicalAggregate aggregate = (LogicalAggregate) finalInput; + List outputExpressions = aggregate.getOutputExpressions(); + for (NamedExpression outputExpr : outputExpressions) { + if (!outputExpr.anyMatch(expr -> expr instanceof AggregateFunction)) { + outputSlots.add(outputExpr.toSlot()); + } + } + } + Scope outputWithoutAggFunc = toScope(cascadesContext, outputSlots.build()); + SimpleExprAnalyzer bindInInputChildScope = buildCustomSlotBinderAnalyzer( + sort, cascadesContext, inputScope, true, false, + (analyzer, unboundSlot) -> { + if (finalInput instanceof LogicalAggregate) { + List boundInOutputWithoutAggFunc = analyzer.bindSlotByScope(unboundSlot, + outputWithoutAggFunc); + if (!boundInOutputWithoutAggFunc.isEmpty()) { + return ImmutableList.of(boundInOutputWithoutAggFunc.get(0)); + } + } + return analyzer.bindExactSlotsByThisScope(unboundSlot, inputChildrenScope.get()); + }); + return bindInInputChildScope; + } }