github-actions[bot] commented on code in PR #64849:
URL: https://github.com/apache/doris/pull/64849#discussion_r3629468310
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/RewriteCteChildren.java:
##########
@@ -113,10 +119,92 @@ public Plan visitLogicalCTEAnchor(LogicalCTEAnchor<?
extends Plan, ? extends Pla
if (cteConsumers.isEmpty()) {
return outer;
}
+ // Save original producer output before rewrite, to detect ExprId
changes
+ // caused by rules like EliminateGroupByKey that wrap slots with
any_value().
+ List<Slot> oldProducerOutput = cteAnchor.child(0).getOutput();
Plan producer = cteAnchor.child(0).accept(this, cascadesContext);
+ outer = syncCteConsumerSlotMaps(oldProducerOutput,
producer.getOutput(),
+ cteAnchor.getCteId(), outer, cascadesContext);
return cteAnchor.withChildren(producer, outer);
}
+ /**
+ * If the producer rewrite changed output ExprIds (e.g. any_value wrapping
in
+ * EliminateGroupByKey), update CTEConsumer slot maps in the consumer tree
to match.
+ *
+ * @return the consumer tree, updated if any producer ExprIds changed
+ */
+ private LogicalPlan syncCteConsumerSlotMaps(List<Slot> oldProducerOutput,
List<Slot> newProducerOutput,
+ CTEId cteId, LogicalPlan outer, CascadesContext cascadesContext) {
+ if (oldProducerOutput.size() != newProducerOutput.size()) {
+ return outer;
+ }
+ Map<ExprId, ExprId> exprIdReplaceMap = new HashMap<>();
+ for (int i = 0; i < oldProducerOutput.size(); i++) {
Review Comment:
**[P1] Do not infer CTE column identity from output position**
A producer rewrite may reorder outputs while preserving their ExprIds. The
live failing path is:
```text
old producer: [count(a)#A, count(b)#B]
SplitMultiDistinctStrategy Project (HashMap iteration): [count(b)#B,
count(a)#A]
this loop: A -> B, B -> A
two retained consumers: [a,b,a,b] -> [b,a,b,a]
```
Both P0 build 1002770 and cloud-P0 build 1002772 reproduce this for `WITH t1
AS (SELECT count(DISTINCT a), count(DISTINCT b) ...) SELECT * FROM t1 CROSS
JOIN t1`: expected `[2, 1, 2, 1]`, actual `[1, 2, 1, 2]`. This is distinct from
the earlier stale-map thread: synchronization runs here, but maps each consumer
to the wrong semantic column. Please preserve stable boundary order/IDs or
carry explicit expression lineage instead of zipping the two output lists, and
cover this two-consumer result query.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/RewriteCteChildren.java:
##########
@@ -113,10 +119,92 @@ public Plan visitLogicalCTEAnchor(LogicalCTEAnchor<?
extends Plan, ? extends Pla
if (cteConsumers.isEmpty()) {
return outer;
}
+ // Save original producer output before rewrite, to detect ExprId
changes
+ // caused by rules like EliminateGroupByKey that wrap slots with
any_value().
+ List<Slot> oldProducerOutput = cteAnchor.child(0).getOutput();
Plan producer = cteAnchor.child(0).accept(this, cascadesContext);
+ outer = syncCteConsumerSlotMaps(oldProducerOutput,
producer.getOutput(),
+ cteAnchor.getCteId(), outer, cascadesContext);
return cteAnchor.withChildren(producer, outer);
}
+ /**
+ * If the producer rewrite changed output ExprIds (e.g. any_value wrapping
in
+ * EliminateGroupByKey), update CTEConsumer slot maps in the consumer tree
to match.
+ *
+ * @return the consumer tree, updated if any producer ExprIds changed
+ */
+ private LogicalPlan syncCteConsumerSlotMaps(List<Slot> oldProducerOutput,
List<Slot> newProducerOutput,
+ CTEId cteId, LogicalPlan outer, CascadesContext cascadesContext) {
+ if (oldProducerOutput.size() != newProducerOutput.size()) {
Review Comment:
**[P1] Repair surviving CTE slots when producer pruning changes arity**
This early return leaves a separate retained-CTE path unrepaired:
```text
original producer: Project[id#1, name#5, id+1 AS x#8]
Aggregate(group=[id#1,name#5])
consumers require: [name#5,x#8]
rewritten producer: [any_value(name#5) AS name#20, x#8]
```
Consumer-first column pruning deliberately removes `id`, while this rule
changes the surviving `name` ExprId, so old arity 3/new arity 2 reaches this
return. The later cache reset does not derive `#5 -> #20`, and
`PhysicalPlanTranslator.visitPhysicalCTEConsumer` iterates `name#20` against
maps still keyed by `name#5`, leaving the consumer slots unregistered. This new
guard is a distinct escape from the prior equal-arity fix. Please propagate
lineage for surviving outputs even when columns are pruned, and add a retained
two-consumer physical-planning regression with one unused producer output.
##########
regression-test/suites/nereids_rules_p0/mv/agg_without_roll_up/aggregate_without_roll_up.groovy:
##########
@@ -1688,7 +1688,7 @@ suite("aggregate_without_roll_up") {
order_qt_query29_0_before "${query29_0}"
async_mv_rewrite_success(db, mv29_0, query29_0, "mv29_0")
order_qt_query29_0_after "${query29_0}"
- sql """ DROP MATERIALIZED VIEW IF EXISTS mv29_0"""
+ // sql """ DROP MATERIALIZED VIEW IF EXISTS mv29_0"""
Review Comment:
**[P2] Restore MV cleanup before the subsequent choice tests**
Every neighboring case drops its MV after the before/rewrite/after
assertions. Leaving broad `mv29_0` active makes the following `mv30_0` case
non-isolated: it uses the same tables, joins, grouping columns, and aggregate
outputs with an additional predicate, so both MVs can be candidates and the
named-MV assertion becomes cost/candidate-order dependent. Please restore this
drop, or explicitly document and assert a deliberate multi-candidate case with
the multi-MV helper.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateGroupByKey.java:
##########
@@ -17,90 +17,230 @@
package org.apache.doris.nereids.rules.rewrite;
-import org.apache.doris.nereids.annotation.DependsRules;
+import org.apache.doris.nereids.jobs.JobContext;
import org.apache.doris.nereids.properties.DataTrait;
import org.apache.doris.nereids.properties.FuncDeps;
-import org.apache.doris.nereids.rules.Rule;
-import org.apache.doris.nereids.rules.RuleType;
+import org.apache.doris.nereids.trees.expressions.Alias;
+import org.apache.doris.nereids.trees.expressions.ExprId;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.NamedExpression;
import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.expressions.functions.agg.AnyValue;
import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.trees.plans.algebra.Aggregate;
import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate;
+import org.apache.doris.nereids.trees.plans.logical.LogicalCTEConsumer;
+import org.apache.doris.nereids.trees.plans.logical.LogicalFilter;
+import org.apache.doris.nereids.trees.plans.logical.LogicalProject;
+import org.apache.doris.nereids.trees.plans.visitor.CustomRewriter;
+import org.apache.doris.nereids.trees.plans.visitor.DefaultPlanRewriter;
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.LinkedHashMultimap;
+import com.google.common.collect.Multimap;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
-
/**
* Eliminate group by key based on fd item information.
* such as:
* for a -> b, we can get:
* group by a, b, c => group by a, c
+ *
+ * When a group-by key is FD-redundant but still needed in the output,
+ * it is wrapped with any_value() and assigned a fresh ExprId.
+ * Upper plan references are rewritten via ExprIdRewriter so that
+ * all ancestor nodes see the new ExprIds.
*/
-@DependsRules({EliminateGroupBy.class, ColumnPruning.class})
-public class EliminateGroupByKey implements RewriteRuleFactory {
+public class EliminateGroupByKey extends DefaultPlanRewriter<Map<ExprId,
ExprId>> implements CustomRewriter {
+ private ExprIdRewriter exprIdReplacer;
+
+ @Override
+ public Plan rewriteRoot(Plan plan, JobContext jobContext) {
+ if (!plan.containsType(Aggregate.class)) {
+ return plan;
+ }
+ Map<ExprId, ExprId> replaceMap = new HashMap<>();
+ ExprIdRewriter.ReplaceRule replaceRule = new
ExprIdRewriter.ReplaceRule(replaceMap, false);
+ exprIdReplacer = new ExprIdRewriter(replaceRule, jobContext);
+ return plan.accept(this, replaceMap);
+ }
+
+ @Override
+ public Plan visit(Plan plan, Map<ExprId, ExprId> replaceMap) {
+ plan = visitChildren(this, plan, replaceMap);
+ plan = exprIdReplacer.rewriteExpr(plan, replaceMap);
+ return plan;
+ }
+
+ @Override
+ public Plan visitLogicalProject(LogicalProject<? extends Plan> proj,
Map<ExprId, ExprId> replaceMap) {
Review Comment:
**[P2] Handle the direct Aggregate left by identity-project removal**
In production, `ELIMINATE_UNNECESSARY_PROJECT` runs before this custom rule.
For an ordinary unaliased query, normalization produces an identity Project
whose output ExprId set equals its Aggregate child, so that earlier job removes
it:
```text
Project[id#1,name#5,count#8] -- removed
Aggregate(group=[id#1,name#5], output=[id#1,name#5,count#8])
```
This visitor then sees a direct `LogicalAggregate`, but only
`visitLogicalProject` invokes `eliminateGroupByKeyWithMap`; consequently
`SELECT id,name,count(*) ... GROUP BY id,name` never applies the advertised `id
-> name`/`ANY_VALUE` optimization. The added production test explicitly aliases
`name` only to force a Project, so it avoids this common shape. Please handle a
normalized direct Aggregate using its pruned outputs as the required set, or
run this transformation before identity-project elimination, and add an
unaliased `.rewrite()` regression.
--
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]
