github-actions[bot] commented on code in PR #67362: URL: https://github.com/apache/doris/pull/67362#discussion_r3901744663
########## regression-test/suites/nereids_rules_p0/mv/agg_without_roll_up/aggregate_without_roll_up_projection.groovy: ########## @@ -0,0 +1,59 @@ +// 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("aggregate_without_roll_up_projection") { + String db = context.config.getDbNameByFile(context.file) + sql "use ${db}" Review Comment: Please pin the session time zone here. `SessionVariable.timeZone` defaults to the FE process time zone, but this test inserts offset-less TIMESTAMPTZ values and its golden output hard-codes `+08:00`; on a UTC runner the rendered values use `+00:00` and the regression fails for environmental reasons. ```suggestion sql "use ${db}" sql "set time_zone = '+08:00'" ``` ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java: ########## @@ -265,7 +267,83 @@ protected LogicalAggregate<Plan> aggregateRewriteByView( queryAggregate.getSourceRepeat().get().getRepeatType(), tempRewritedPlan); return NormalizeRepeat.doNormalize(repeat); } - return new LogicalAggregate<>(finalGroupExpressions, finalOutputExpressions, tempRewritedPlan); + + // The rewritten aggregate should group by the query bottom aggregate's group by expressions, + // and its output expressions should be the rewritten group by expressions and the rolled up + // aggregate functions. The query top plan output expressions are recomputed by a project above + // the rewritten aggregate, so the projection of a group by key in the query top plan (such as + // `select cast(date_trunc(ts, 'day') as string) from t group by date_trunc(ts, 'day')`) will not + // be wrongly treated as a group by key of the rewritten aggregate. + // The mapping from the query bottom aggregate output slot to the new aggregate output expression + // is used to rewrite the query top plan output expressions to reference the new aggregate output. + Map<Expression, Expression> bottomAggOutputToNewExprMap = new HashMap<>(); + Set<Expression> queryGroupByExpressionSet = new HashSet<>(queryGroupByExpressions); + for (NamedExpression queryAggregateOutput : queryAggregate.getOutputExpressions()) { + // The shuttled query bottom aggregate output is used as the map key, so that the shuttled + // query top plan output expressions can be rewritten to reference the new aggregate output. + Expression shuttledQueryAggregateOutput = ExpressionUtils.shuttleExpressionWithLineage( + queryAggregateOutput, queryTopPlan); + if (queryGroupByExpressionSet.contains(queryAggregateOutput)) { + // if it is a group by expression, rewrite it to the new aggregate group by key + Expression rewrittenGroupByExpression = tryRewriteExpression(queryStructInfo, + queryAggregateOutput, mvExprToMvScanExprQueryBased, groupByMode, materializationContext, + "View dimensions doesn't not cover the query dimensions", + () -> String.format("mvExprToMvScanExprQueryBased is %s,\n queryExpression is %s", + mvExprToMvScanExprQueryBased, queryAggregateOutput)); + if (rewrittenGroupByExpression == null) { + return null; + } + NamedExpression groupByOutput = rewrittenGroupByExpression instanceof NamedExpression + ? (NamedExpression) rewrittenGroupByExpression : new Alias(rewrittenGroupByExpression); + finalGroupExpressions.add(groupByOutput); + finalOutputExpressions.add(groupByOutput); + bottomAggOutputToNewExprMap.put(shuttledQueryAggregateOutput, groupByOutput.toSlot()); + } else { + // if it is an aggregate function, try to roll up and rewrite + Expression rewrittenFunction = tryRewriteExpression(queryStructInfo, queryAggregateOutput, + mvExprToMvScanExprQueryBased, aggregateFunctionMode, materializationContext, + "Query function roll up fail", + () -> String.format("queryExpression = %s,\n mvExprToMvScanExprQueryBased = %s", + queryAggregateOutput, mvExprToMvScanExprQueryBased)); + if (rewrittenFunction == null) { + return null; + } + NamedExpression functionOutput = new Alias(rewrittenFunction); + finalOutputExpressions.add(functionOutput); + bottomAggOutputToNewExprMap.put(shuttledQueryAggregateOutput, functionOutput.toSlot()); + } + } + + LogicalAggregate<Plan> rewrittenAggregate = + new LogicalAggregate<>(finalGroupExpressions, finalOutputExpressions, tempRewritedPlan); + + // rewrite the query top plan output expressions to reference the rewritten aggregate output, + // the query top plan output slot is shuttled by lineage firstly to restore the projection + // expression, so a projection of the group by key in the query top plan can be recomputed + // by a project above the rewritten aggregate. + List<NamedExpression> topProjectExpressions = new ArrayList<>(); + for (Expression topExpression : queryTopPlan.getOutput()) { + Expression shuttledTopExpression = ExpressionUtils.shuttleExpressionWithLineage( + topExpression, queryTopPlan); + Expression replacedExpression = ExpressionUtils.replace(shuttledTopExpression, + bottomAggOutputToNewExprMap); + topProjectExpressions.add(replacedExpression instanceof NamedExpression + ? (NamedExpression) replacedExpression : new Alias(replacedExpression)); + } + // If the query top plan output expressions can be produced by the rewritten aggregate directly, + // return the aggregate, otherwise compute them by a project above the rewritten aggregate. + boolean needTopProject = false; Review Comment: The top project can be a strict matching prefix of the normalized aggregate outputs. For `SELECT k1 FROM t GROUP BY k1, k2`, normalization produces `Project(k1) -> Aggregate(output=[k1,k2])`; this code rebuilds `[mv_k1,mv_k2]`, compares only index 0, and returns the wider aggregate. `MaterializedViewUtils.normalizeExpressions` then rejects the candidate on its output-count check, so a valid sync MV (and async roll-up) is silently not used. ```suggestion boolean needTopProject = topProjectExpressions.size() != finalOutputExpressions.size(); ``` Please also add a leading-subset regression for this boundary. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
