[GitHub] [doris] morrySnow commented on a diff in pull request #21067: [Feature](materialized view) support query match mv with agg_state on nereids planner

["morrySnow (via GitHub)" <gi...@apache.org>]

morrySnow commented on code in PR #21067:
URL: https://github.com/apache/doris/pull/21067#discussion_r1247533577


##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -251,6 +252,21 @@ public static Optional<Slot> extractSlotOrCastOnSlot(Expression expr) {
         }
     }
 
+    /**
+     * get all slot child of expr
+     */
+    public static List<Slot> getAllSlot(Expression expr) {

Review Comment:
   u could use `Expression.collect()` directly: `expr.collect(SlotRefenrece.class::isInstance)`



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/mv/SelectMaterializedIndexWithAggregate.java:
##########
@@ -1391,6 +1395,36 @@ public Expression visitNdv(Ndv ndv, RewriteContext context) {
             }
             return ndv;
         }
+
+        /**
+         * agg(col) -> agg_merge(mva_generic_aggregation__agg_state(col)) eg: max_by(k2,
+         * k3) -> max_by_merge(mva_generic_aggregation__max_by_state(k2, k3))
+         */
+        @Override
+        public Expression visitAggregateFunction(AggregateFunction aggregateFunction, RewriteContext context) {

Review Comment:
   we have override some agg function in this Rewriter, such as `ndv`. So, if we have a `ndv(xx)` in sql, then it cannot processed by this function by default. if u want `visitAggregateFunction` process all aggregate function, u should call this function in `visitXXX` in this Rewriter explicitly. for example
   ```java
   public Expression visitNdv(Ndv ndv, RewriteContext context) {
       Expression newNdv = visitAggregateFunction(ndv, context);
       if (newNdv != ndv) {
           return newNdv;
       }
       ...
   }



##########
fe/fe-core/src/main/java/org/apache/doris/analysis/NativeInsertStmt.java:
##########
@@ -437,9 +437,7 @@ private void analyzeSubquery(Analyzer analyzer) throws UserException {
                 mentionedColumns.add(col.getName());
                 targetColumns.add(col);
             }
-            realTargetColumnNames = targetColumns.stream().map(column -> column.getName()).collect(Collectors.toList());
         } else {
-            realTargetColumnNames = targetColumnNames;

Review Comment:
   if the logical of insert changed, please ensure Nereids' insert could be work well, to test Nereids' dml, set session variable 'enable_nereids_dml' to `true`



##########
fe/fe-core/src/main/java/org/apache/doris/analysis/NativeInsertStmt.java:
##########
@@ -453,8 +451,8 @@ private void analyzeSubquery(Analyzer analyzer) throws UserException {
             // hll column mush in mentionedColumns
             for (Column col : targetTable.getBaseSchema()) {
                 if (col.getType().isObjectStored() && !mentionedColumns.contains(col.getName())) {
-                    throw new AnalysisException(" object-stored column " + col.getName()
-                            + " must in insert into columns");
+                    throw new AnalysisException(
+                            " object-stored column " + col.getName() + " mush in insert into columns");

Review Comment:
   why change this? BTW, typo: must -> mush



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org