[Enchancement](Materialized-View) forbiden some case in create mv wit…

…h group by and fix select fail on g… (apache#16820) 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
yujun777 · Feb 20, 2023 · ce3afe7 · ce3afe7
1 parent 1011422
commit ce3afe7
Show file tree

Hide file tree

Showing 9 changed files with 103 additions and 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
@@ -158,6 +158,7 @@ public void analyze(Analyzer analyzer) throws UserException {
                     + 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 @@ private void analyzeFromClause() throws AnalysisException {
         dbName = tableName.getDb();
     }
 
+    private void analyzeGroupByClause() throws AnalysisException {
+        if (selectStmt.getGroupByClause() == null) {
+            return;
+        }
+        List<Expr> groupingExprs = selectStmt.getGroupByClause().getGroupingExprs();
+        List<FunctionCallExpr> aggregateExprs = selectStmt.getAggInfo().getAggregateExprs();
+        List<Expr> 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 static String oldmvColumnBreaker(String name) {
         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
@@ -46,6 +46,7 @@
 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 @@ private void analyzeAggregation(Analyzer analyzer) throws AnalysisException {
         // 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<TupleId> tupleIds = Lists.newArrayList();
+                    List<SlotId> 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
@@ -259,6 +259,11 @@ private Expr matchExpr(Expr expr, Analyzer analyzer, OlapTable olapTable,
 
         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
@@ -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
@@ -561,6 +561,8 @@ public void testMVColumnsWithoutOrderby(@Injectable SlotRef slotRef1,
                 result = columnName3;
                 slotRef4.toSql();
                 result = columnName4;
+                selectStmt.getGroupByClause();
+                result = null;
             }
         };
 
@@ -664,6 +666,8 @@ public void testMVColumnsWithoutOrderbyWithoutAggregation(@Injectable SlotRef sl
                 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 void testMVColumnsWithoutOrderbyWithoutAggregationWithFloat(@Injectable S
                 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 void testMVColumnsWithoutOrderbyWithoutAggregationWithVarchar(@Injectable
                 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 void testMVColumnsWithFirstVarchar(@Injectable SlotRef slotRef1,
                 result = columnName1;
                 slotRef1.getType().getPrimitiveType();
                 result = PrimitiveType.VARCHAR;
+                selectStmt.getGroupByClause();
+                result = null;
             }
         };
 
@@ -1040,6 +1050,8 @@ public void testMVColumns(@Injectable SlotRef slotRef1,
                 result = columnName1;
                 slotRef2.getColumnName();
                 result = columnName2;
+                selectStmt.getGroupByClause();
+                result = null;
             }
         };
 
@@ -1095,6 +1107,8 @@ public void testDeduplicateMV(@Injectable SlotRef slotRef1,
                 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
@@ -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
@@ -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
@@ -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/...ion-test/suites/materialized_view_p0/multi_slot_k1a2p2ap3ps/multi_slot_k1a2p2ap3ps.groovy b/...ion-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;"
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -23,3 +23,6 @@ @@
 	2
 	-3
+    -- !select_mv --
+    \N	-4