From 214978cb7b29e8366de2a9665e9b36d6f43bdaa5 Mon Sep 17 00:00:00 2001 From: spaces-x Date: Tue, 14 Jun 2022 15:09:33 +0800 Subject: [PATCH 1/4] [fix](groupingsets) deep check in the SelectStmt analyze that grouping set columns cannot be input parameters in aggregate functions. --- .../org/apache/doris/analysis/SelectStmt.java | 43 ++++++++++++++----- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java index 067a8882debe1a..25437779dfceab 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java @@ -60,8 +60,10 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Queue; import java.util.Set; import java.util.UUID; import java.util.stream.Collectors; @@ -439,17 +441,7 @@ public void analyze(Analyzer analyzer) throws UserException { } } if (groupByClause != null && groupByClause.isGroupByExtension()) { - for (SelectListItem item : selectList.getItems()) { - if (item.getExpr() instanceof FunctionCallExpr && item.getExpr().fn instanceof AggregateFunction) { - for (Expr expr : groupByClause.getGroupingExprs()) { - if (item.getExpr().contains(expr)) { - throw new AnalysisException("column: " + expr.toSql() + " cannot both in select list and " - + "aggregate functions when using GROUPING SETS/CUBE/ROLLUP, please use union" - + " instead."); - } - } - } - } + checkSelectItemsForGroupingSet(); groupingInfo = new GroupingInfo(analyzer, groupByClause); groupingInfo.substituteGroupingFn(resultExprs, analyzer); } else { @@ -562,6 +554,35 @@ public void analyze(Analyzer analyzer) throws UserException { } } + public void checkSelectItemsForGroupingSet() throws AnalysisException { + for (SelectListItem item : selectList.getItems()) { + Expr selectExprRoot = item.getExpr(); + List aggFunctions = getAggFuncExprsFromChildren(selectExprRoot); + for (Expr aggFunction : aggFunctions) { + for (Expr groupingExpr : groupByClause.getGroupingExprs()) { + if (aggFunction.contains(groupingExpr)) { + throw new AnalysisException("column: " + groupingExpr.toSql() + " cannot both in select list and " + + "aggregate functions when using GROUPING SETS/CUBE/ROLLUP, please use union" + + " instead."); + } + } + } + } + } + + public List getAggFuncExprsFromChildren(Expr expr) { + List aggFuncExprs = new LinkedList<>(); + Queue exprsQueue = new LinkedList<>(); + exprsQueue.offer(expr); + while (!exprsQueue.isEmpty()) { + Expr exprChild = exprsQueue.poll(); + if (exprChild instanceof FunctionCallExpr && exprChild.getFn() instanceof AggregateFunction) { + aggFuncExprs.add(exprChild); + } + exprsQueue.addAll(exprChild.getChildrenWithoutCast()); + } + return aggFuncExprs; + } public List getTableRefIds() { List result = Lists.newArrayList(); From 24474a0c600c5b04c8e6345a3906bd6eb7271430 Mon Sep 17 00:00:00 2001 From: spaces-x Date: Wed, 15 Jun 2022 14:51:00 +0800 Subject: [PATCH 2/4] add UT & apply code-format --- .../org/apache/doris/analysis/SelectStmt.java | 16 +++++++++++++--- .../org/apache/doris/planner/PlannerTest.java | 11 +++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java index 25437779dfceab..ff7670b85a2b98 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java @@ -554,6 +554,11 @@ public void analyze(Analyzer analyzer) throws UserException { } } + /** + * check whether grouping set columns are in the agg function + * within the select items. If true, throw an AnalysisException. + * @throws AnalysisException + */ public void checkSelectItemsForGroupingSet() throws AnalysisException { for (SelectListItem item : selectList.getItems()) { Expr selectExprRoot = item.getExpr(); @@ -561,15 +566,20 @@ public void checkSelectItemsForGroupingSet() throws AnalysisException { for (Expr aggFunction : aggFunctions) { for (Expr groupingExpr : groupByClause.getGroupingExprs()) { if (aggFunction.contains(groupingExpr)) { - throw new AnalysisException("column: " + groupingExpr.toSql() + " cannot both in select list and " - + "aggregate functions when using GROUPING SETS/CUBE/ROLLUP, please use union" - + " instead."); + throw new AnalysisException("column: " + groupingExpr.toSql() + " cannot both in" + + " select list and aggregate functions when using GROUPING SETS/CUBE/ROLLUP," + + " please use union instead."); } } } } } + /** + * Get all AggregateFunctions,which are under the `expr` in the Expr-tree. + * @param expr + * @return + */ public List getAggFuncExprsFromChildren(Expr expr) { List aggFuncExprs = new LinkedList<>(); Queue exprsQueue = new LinkedList<>(); diff --git a/fe/fe-core/src/test/java/org/apache/doris/planner/PlannerTest.java b/fe/fe-core/src/test/java/org/apache/doris/planner/PlannerTest.java index 73fe9bf24bfddc..11eed15fbbc229 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/planner/PlannerTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/planner/PlannerTest.java @@ -452,6 +452,17 @@ public void testStringType() { Assertions.assertTrue(exception.getMessage().contains("String Type should not be used in key column[k1].")); } + @Test + public void testSelectAggregateItemCheckOnGroupingSet() throws Exception { + String sql = "explain select k1,if(k2=null, null, count(distinct k2)) from db1.tbl4" + + " group by grouping sets((k1),(k1,k2))"; + String errorMessage = "errCode = 2, detailMessage = column: `k2` cannot both in select list and " + + "aggregate functions when using GROUPING SETS/CUBE/ROLLUP, please use union instead."; + StmtExecutor stmtExecutor = new StmtExecutor(connectContext, sql); + stmtExecutor.execute(); + Assertions.assertTrue(connectContext.getState().getErrorMessage().contains(errorMessage)); + } + @Test public void testPushDownPredicateOnGroupingSetAggregate() throws Exception { String sql = "explain select k1, k2, count(distinct v1) from db1.tbl4" From f1c1808bd8caaea76469982fdf1f435a6ff404b1 Mon Sep 17 00:00:00 2001 From: spaces-x Date: Wed, 15 Jun 2022 16:55:26 +0800 Subject: [PATCH 3/4] apply code-format --- .../java/org/apache/doris/analysis/SelectStmt.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java index ff7670b85a2b98..6ba1ee09092681 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java @@ -557,6 +557,7 @@ public void analyze(Analyzer analyzer) throws UserException { /** * check whether grouping set columns are in the agg function * within the select items. If true, throw an AnalysisException. + * * @throws AnalysisException */ public void checkSelectItemsForGroupingSet() throws AnalysisException { @@ -566,9 +567,9 @@ public void checkSelectItemsForGroupingSet() throws AnalysisException { for (Expr aggFunction : aggFunctions) { for (Expr groupingExpr : groupByClause.getGroupingExprs()) { if (aggFunction.contains(groupingExpr)) { - throw new AnalysisException("column: " + groupingExpr.toSql() + " cannot both in" + - " select list and aggregate functions when using GROUPING SETS/CUBE/ROLLUP," + - " please use union instead."); + throw new AnalysisException("column: " + groupingExpr.toSql() + " cannot both in" + + " select list and aggregate functions when using GROUPING SETS/CUBE/ROLLUP," + + " please use union instead."); } } } @@ -577,8 +578,10 @@ public void checkSelectItemsForGroupingSet() throws AnalysisException { /** * Get all AggregateFunctions,which are under the `expr` in the Expr-tree. + * * @param expr - * @return + * + * @return list of exprs */ public List getAggFuncExprsFromChildren(Expr expr) { List aggFuncExprs = new LinkedList<>(); From ceaee8fe1ac0992a66855e49129a50c3fb0c2963 Mon Sep 17 00:00:00 2001 From: spaces-x Date: Thu, 16 Jun 2022 10:57:54 +0800 Subject: [PATCH 4/4] apply code-format --- .../src/main/java/org/apache/doris/analysis/SelectStmt.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java index 6ba1ee09092681..8ff2b7a3bbc2a5 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java @@ -558,7 +558,7 @@ public void analyze(Analyzer analyzer) throws UserException { * check whether grouping set columns are in the agg function * within the select items. If true, throw an AnalysisException. * - * @throws AnalysisException + * @throws AnalysisException when check failed */ public void checkSelectItemsForGroupingSet() throws AnalysisException { for (SelectListItem item : selectList.getItems()) {