From ce3afe7f13cb8bc00bae679b3e2f48d19e620c78 Mon Sep 17 00:00:00 2001 From: Pxl Date: Mon, 20 Feb 2023 13:04:50 +0800 Subject: [PATCH] =?UTF-8?q?[Enchancement](Materialized-View)=20forbiden=20?= =?UTF-8?q?some=20case=20in=20create=20mv=20with=20group=20by=20and=20fix?= =?UTF-8?q?=20select=20fail=20on=20g=E2=80=A6=20(#16820)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. forbiden some case in create mv with group by select k1+1,sum(abs(k2+2)+k3+3) from d_table group by k1; 2. fix select fail on grouping column have diffrent expr with select list create materialized view k1p2ap3psg as select k1+1,sum(abs(k2+2)+k3+3) from d_table group by k1+1; mysql [test]>explain select k1+1,sum(abs(k2+2)+k3+3) from d_table group by k1; ERROR 1105 (HY000): errCode = 2, detailMessage = select list expression not produced by aggregation output (missing from GROUP BY clause?): `k1` + 1 --- .../analysis/CreateMaterializedViewStmt.java | 42 ++++++++++++++++++- .../org/apache/doris/analysis/SelectStmt.java | 17 ++++++-- .../rewrite/mvrewrite/ExprToSlotRefRule.java | 5 +++ .../mvrewrite/MVSelectFailedException.java | 4 +- .../CreateMaterializedViewStmtTest.java | 14 +++++++ .../agg_have_dup_base/agg_have_dup_base.out | 3 ++ .../multi_slot_k1a2p2ap3ps.out | 5 +++ .../agg_have_dup_base.groovy | 6 +++ .../multi_slot_k1a2p2ap3ps.groovy | 14 +++++++ 9 files changed, 103 insertions(+), 7 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateMaterializedViewStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateMaterializedViewStmt.java index 76c86c62b3..e6c5a00793 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateMaterializedViewStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateMaterializedViewStmt.java @@ -158,6 +158,7 @@ public class CreateMaterializedViewStmt extends DdlStmt { + selectStmt.getHavingPred().toSql()); } analyzeOrderByClause(); + analyzeGroupByClause(); if (selectStmt.getLimit() != -1) { throw new AnalysisException("The limit clause is not supported in add materialized view clause, expr:" + " limit " + selectStmt.getLimit()); @@ -239,6 +240,39 @@ public class CreateMaterializedViewStmt extends DdlStmt { dbName = tableName.getDb(); } + private void analyzeGroupByClause() throws AnalysisException { + if (selectStmt.getGroupByClause() == null) { + return; + } + List groupingExprs = selectStmt.getGroupByClause().getGroupingExprs(); + List aggregateExprs = selectStmt.getAggInfo().getAggregateExprs(); + List selectExprs = selectStmt.getSelectList().getExprs(); + for (Expr expr : selectExprs) { + boolean match = false; + String lhs = selectStmt.getExprFromAliasSMap(expr).toSqlWithoutTbl(); + for (Expr groupExpr : groupingExprs) { + String rhs = selectStmt.getExprFromAliasSMap(groupExpr).toSqlWithoutTbl(); + if (lhs.equalsIgnoreCase(rhs)) { + match = true; + break; + } + } + if (!match) { + for (Expr groupExpr : aggregateExprs) { + String rhs = selectStmt.getExprFromAliasSMap(groupExpr).toSqlWithoutTbl(); + if (lhs.equalsIgnoreCase(rhs)) { + match = true; + break; + } + } + } + + if (!match) { + throw new AnalysisException("The select expr " + lhs + " not in grouping or aggregate columns"); + } + } + } + private void analyzeOrderByClause() throws AnalysisException { if (selectStmt.getOrderByElements() == null) { supplyOrderColumn(); @@ -513,16 +547,20 @@ public class CreateMaterializedViewStmt extends DdlStmt { return name; } + private static boolean mvMatch(String name, String prefix) { + return MaterializedIndexMeta.normalizeName(name).startsWith(prefix); + } + public static boolean isMVColumn(String name) { return isMVColumnAggregate(name) || isMVColumnNormal(name); } public static boolean isMVColumnAggregate(String name) { - return name.startsWith(MATERIALIZED_VIEW_AGGREGATE_NAME_PREFIX); + return mvMatch(name, MATERIALIZED_VIEW_AGGREGATE_NAME_PREFIX); } public static boolean isMVColumnNormal(String name) { - return name.startsWith(MATERIALIZED_VIEW_NAME_PREFIX); + return mvMatch(name, MATERIALIZED_VIEW_NAME_PREFIX); } @Override 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 091db335cd..9363abca26 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 @@ -46,6 +46,7 @@ import org.apache.doris.common.util.SqlUtils; import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.qe.ConnectContext; import org.apache.doris.rewrite.ExprRewriter; +import org.apache.doris.rewrite.mvrewrite.MVSelectFailedException; import org.apache.doris.thrift.TExprOpcode; import com.google.common.base.Preconditions; @@ -1491,9 +1492,19 @@ public class SelectStmt extends QueryStmt { // check that all post-agg exprs point to agg output for (int i = 0; i < selectList.getItems().size(); ++i) { if (!resultExprs.get(i).isBoundByTupleIds(groupingByTupleIds)) { - throw new AnalysisException( - "select list expression not produced by aggregation output " + "(missing from " - + "GROUP BY clause?): " + selectList.getItems().get(i).getExpr().toSql()); + if (CreateMaterializedViewStmt.isMVColumn(resultExprs.get(i).toSqlWithoutTbl())) { + List tupleIds = Lists.newArrayList(); + List slotIds = Lists.newArrayList(); + resultExprs.get(i).getIds(tupleIds, slotIds); + for (TupleId id : tupleIds) { + updateDisableTuplesMVRewriter(id); + } + throw new MVSelectFailedException("Materialized View rewrite invalid"); + } else { + throw new AnalysisException( + "select list expression not produced by aggregation output " + "(missing from " + + "GROUP BY clause?): " + selectList.getItems().get(i).getExpr().toSql()); + } } } if (orderByElements != null) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/rewrite/mvrewrite/ExprToSlotRefRule.java b/fe/fe-core/src/main/java/org/apache/doris/rewrite/mvrewrite/ExprToSlotRefRule.java index ea03d0af62..688600fbdc 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/rewrite/mvrewrite/ExprToSlotRefRule.java +++ b/fe/fe-core/src/main/java/org/apache/doris/rewrite/mvrewrite/ExprToSlotRefRule.java @@ -259,6 +259,11 @@ public class ExprToSlotRefRule implements ExprRewriteRule { if (mvColumn == null) { return expr; + } else { + Expr rhs = mvColumn.getDefineExpr(); + if (rhs == null || !rhs.getClass().equals(expr.getClass())) { + return expr; + } } return rewriteExpr(tableName, mvColumn, analyzer); diff --git a/fe/fe-core/src/main/java/org/apache/doris/rewrite/mvrewrite/MVSelectFailedException.java b/fe/fe-core/src/main/java/org/apache/doris/rewrite/mvrewrite/MVSelectFailedException.java index 286fc71067..4a355885a1 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/rewrite/mvrewrite/MVSelectFailedException.java +++ b/fe/fe-core/src/main/java/org/apache/doris/rewrite/mvrewrite/MVSelectFailedException.java @@ -17,9 +17,9 @@ package org.apache.doris.rewrite.mvrewrite; -import org.apache.doris.common.UserException; +import org.apache.doris.common.AnalysisException; -public class MVSelectFailedException extends UserException { +public class MVSelectFailedException extends AnalysisException { public MVSelectFailedException(String msg) { super(msg); diff --git a/fe/fe-core/src/test/java/org/apache/doris/analysis/CreateMaterializedViewStmtTest.java b/fe/fe-core/src/test/java/org/apache/doris/analysis/CreateMaterializedViewStmtTest.java index 4762b3c9fe..9652fbd239 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/analysis/CreateMaterializedViewStmtTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/analysis/CreateMaterializedViewStmtTest.java @@ -561,6 +561,8 @@ public class CreateMaterializedViewStmtTest { result = columnName3; slotRef4.toSql(); result = columnName4; + selectStmt.getGroupByClause(); + result = null; } }; @@ -664,6 +666,8 @@ public class CreateMaterializedViewStmtTest { result = 4; selectStmt.getAggInfo(); // return null, so that the mv can be a duplicate mv result = null; + selectStmt.getGroupByClause(); + result = null; } }; @@ -762,6 +766,8 @@ public class CreateMaterializedViewStmtTest { result = true; selectStmt.getAggInfo(); // return null, so that the mv can be a duplicate mv result = null; + selectStmt.getGroupByClause(); + result = null; } }; @@ -860,6 +866,8 @@ public class CreateMaterializedViewStmtTest { result = PrimitiveType.VARCHAR; selectStmt.getAggInfo(); // return null, so that the mv can be a duplicate mv result = null; + selectStmt.getGroupByClause(); + result = null; } }; @@ -964,6 +972,8 @@ public class CreateMaterializedViewStmtTest { result = columnName1; slotRef1.getType().getPrimitiveType(); result = PrimitiveType.VARCHAR; + selectStmt.getGroupByClause(); + result = null; } }; @@ -1040,6 +1050,8 @@ public class CreateMaterializedViewStmtTest { result = columnName1; slotRef2.getColumnName(); result = columnName2; + selectStmt.getGroupByClause(); + result = null; } }; @@ -1095,6 +1107,8 @@ public class CreateMaterializedViewStmtTest { result = null; selectStmt.getHavingPred(); result = null; + selectStmt.getGroupByClause(); + result = null; selectStmt.getLimit(); result = -1; slotRef1.toSql(); diff --git a/regression-test/data/materialized_view_p0/agg_have_dup_base/agg_have_dup_base.out b/regression-test/data/materialized_view_p0/agg_have_dup_base/agg_have_dup_base.out index 815c51b9c2..c59641f5cd 100644 --- a/regression-test/data/materialized_view_p0/agg_have_dup_base/agg_have_dup_base.out +++ b/regression-test/data/materialized_view_p0/agg_have_dup_base/agg_have_dup_base.out @@ -23,3 +23,6 @@ 2 2 3 -3 +-- !select_mv -- +\N -4 + diff --git a/regression-test/data/materialized_view_p0/multi_slot_k1a2p2ap3ps/multi_slot_k1a2p2ap3ps.out b/regression-test/data/materialized_view_p0/multi_slot_k1a2p2ap3ps/multi_slot_k1a2p2ap3ps.out index 5df5415679..a3c0a07a99 100644 --- a/regression-test/data/materialized_view_p0/multi_slot_k1a2p2ap3ps/multi_slot_k1a2p2ap3ps.out +++ b/regression-test/data/materialized_view_p0/multi_slot_k1a2p2ap3ps/multi_slot_k1a2p2ap3ps.out @@ -10,3 +10,8 @@ 3 7 5 9 +-- !select_base -- +1 1 +3 7 +5 9 + diff --git a/regression-test/suites/materialized_view_p0/agg_have_dup_base/agg_have_dup_base.groovy b/regression-test/suites/materialized_view_p0/agg_have_dup_base/agg_have_dup_base.groovy index 452435a077..df0e50cac9 100644 --- a/regression-test/suites/materialized_view_p0/agg_have_dup_base/agg_have_dup_base.groovy +++ b/regression-test/suites/materialized_view_p0/agg_have_dup_base/agg_have_dup_base.groovy @@ -69,4 +69,10 @@ suite ("agg_have_dup_base") { contains "(k12s3m)" } qt_select_mv "select k1,max(k2) from d_table group by k1 order by k1;" + + explain { + sql("select unix_timestamp(k1) tmp,sum(k2) from d_table group by tmp;") + contains "(k12s3m)" + } + qt_select_mv "select unix_timestamp(k1) tmp,sum(k2) from d_table group by tmp order by tmp;" } diff --git a/regression-test/suites/materialized_view_p0/multi_slot_k1a2p2ap3ps/multi_slot_k1a2p2ap3ps.groovy b/regression-test/suites/materialized_view_p0/multi_slot_k1a2p2ap3ps/multi_slot_k1a2p2ap3ps.groovy index fb5f458f14..419ff5f42d 100644 --- a/regression-test/suites/materialized_view_p0/multi_slot_k1a2p2ap3ps/multi_slot_k1a2p2ap3ps.groovy +++ b/regression-test/suites/materialized_view_p0/multi_slot_k1a2p2ap3ps/multi_slot_k1a2p2ap3ps.groovy @@ -36,6 +36,14 @@ suite ("multi_slot_k1a2p2ap3ps") { sql "insert into d_table select 2,2,2,'b';" sql "insert into d_table select 3,-3,null,'c';" + boolean createFail = false; + try { + sql "create materialized view k1a2p2ap3ps as select abs(k1)+k2+1,sum(abs(k2+2)+k3+3) from d_table group by abs(k1)+k2;" + } catch (Exception e) { + createFail = true; + } + assertTrue(createFail); + def result = "null" sql "create materialized view k1a2p2ap3ps as select abs(k1)+k2+1,sum(abs(k2+2)+k3+3) from d_table group by abs(k1)+k2+1;" while (!result.contains("FINISHED")){ @@ -57,4 +65,10 @@ suite ("multi_slot_k1a2p2ap3ps") { contains "(k1a2p2ap3ps)" } qt_select_mv "select abs(k1)+k2+1,sum(abs(k2+2)+k3+3) from d_table group by abs(k1)+k2+1 order by abs(k1)+k2+1;" + + explain { + sql("select abs(k1)+k2+1,sum(abs(k2+2)+k3+3) from d_table group by abs(k1)+k2 order by abs(k1)+k2") + contains "(d_table)" + } + qt_select_base "select abs(k1)+k2+1,sum(abs(k2+2)+k3+3) from d_table group by abs(k1)+k2 order by abs(k1)+k2;" }