github-actions[bot] commented on code in PR #69111:
URL: https://github.com/apache/doris/pull/69111#discussion_r4292465730
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/RewriteCteChildren.java:
##########
@@ -99,22 +109,138 @@ public Plan visitLogicalCTEAnchor(LogicalCTEAnchor<?
extends Plan, ? extends Pla
cascadesContext.addPlanProcesses(outerCascadesCtx.getPlanProcesses());
cascadesContext.getStatementContext().getRewrittenCteConsumer().put(cteAnchor.getCteId(),
outer);
}
+ Set<LogicalCTEConsumer> cteConsumers = collectCteConsumers(outer,
cteAnchor.getCteId());
+ cascadesContext.getCteIdToConsumers().put(cteAnchor.getCteId(),
cteConsumers);
+ if (cteConsumers.isEmpty()) {
+ return outer;
+ }
+ List<Slot> oldProducerOutputs = cteAnchor.child(0).getOutput();
+ Plan producer = cteAnchor.child(0).accept(this, cascadesContext);
+ Map<ExprId, ExprId> producerExprIdRemap =
computeProducerExprIdRemap(oldProducerOutputs, producer.getOutput());
+ if (!producerExprIdRemap.isEmpty()) {
+ // The producer subtree is rewritten in its own context, so a rule
of it may rename the exprIds of the
+ // producer outputs, while the consumers, which are rewritten
before the producer, still map to the
+ // previous exprIds. Refresh both maps of every consumer of this
CTE with the new producer slots.
+ Map<ExprId, Slot> producerOutputSlots =
producer.getOutput().stream()
+ .collect(Collectors.toMap(Slot::getExprId,
Function.identity()));
Review Comment:
[P2] Allow repeated producer ExprIds in the output lookup
A CTE can legally expose the same slot more than once; `LogicalCTEConsumer`
explicitly supports this with its producer-to-consumer multimap. For a
nonunique `t` with varying `v`, consider:
```sql
WITH c AS (
SELECT k,v,v,count(*) cnt,v+1 z
FROM t WHERE k=5 GROUP BY k,v
)
SELECT l.*,r.* FROM c l CROSS JOIN c r;
```
The producer starts as `Project(k#0,v#1,v#1,cnt#2,(v#1+1) AS z#3)` over the
aggregate. `ConstantPropagation.visitLogicalAggregate` gives `k` a fresh ID,
while the computed `z` keeps the repeated-`v` project from being eliminated.
The rewritten producer therefore outputs `[k#fresh,v#1,v#1,cnt#2,z#3]`. The `k`
remap is nonempty, so this new `Collectors.toMap` throws a duplicate-key
`IllegalStateException` on `v#1` before refreshing either consumer. Build the
lookup allowing equivalent repeated IDs and add this integration case. This is
a new exception in the remapping fix, distinct from the earlier
missing-boundary-map report.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/RewriteCteChildren.java:
##########
@@ -99,22 +109,138 @@ public Plan visitLogicalCTEAnchor(LogicalCTEAnchor<?
extends Plan, ? extends Pla
cascadesContext.addPlanProcesses(outerCascadesCtx.getPlanProcesses());
cascadesContext.getStatementContext().getRewrittenCteConsumer().put(cteAnchor.getCteId(),
outer);
}
+ Set<LogicalCTEConsumer> cteConsumers = collectCteConsumers(outer,
cteAnchor.getCteId());
+ cascadesContext.getCteIdToConsumers().put(cteAnchor.getCteId(),
cteConsumers);
+ if (cteConsumers.isEmpty()) {
+ return outer;
+ }
+ List<Slot> oldProducerOutputs = cteAnchor.child(0).getOutput();
+ Plan producer = cteAnchor.child(0).accept(this, cascadesContext);
+ Map<ExprId, ExprId> producerExprIdRemap =
computeProducerExprIdRemap(oldProducerOutputs, producer.getOutput());
+ if (!producerExprIdRemap.isEmpty()) {
+ // The producer subtree is rewritten in its own context, so a rule
of it may rename the exprIds of the
+ // producer outputs, while the consumers, which are rewritten
before the producer, still map to the
+ // previous exprIds. Refresh both maps of every consumer of this
CTE with the new producer slots.
+ Map<ExprId, Slot> producerOutputSlots =
producer.getOutput().stream()
+ .collect(Collectors.toMap(Slot::getExprId,
Function.identity()));
+ outer = (LogicalPlan) outer.<Plan, Void>accept(new
CteConsumerOutputsRefresher(cteAnchor.getCteId(),
+ producerExprIdRemap, producerOutputSlots), null);
+
cascadesContext.getStatementContext().getRewrittenCteConsumer().put(cteAnchor.getCteId(),
outer);
+ cascadesContext.getCteIdToConsumers()
+ .put(cteAnchor.getCteId(), collectCteConsumers(outer,
cteAnchor.getCteId()));
+ }
+ return cteAnchor.withChildren(producer, outer);
+ }
+
+ private static Set<LogicalCTEConsumer> collectCteConsumers(Plan outer,
CTEId cteId) {
Set<LogicalCTEConsumer> cteConsumers = Sets.newHashSet();
outer.foreach(p -> {
if (p instanceof LogicalCTEConsumer) {
LogicalCTEConsumer logicalCTEConsumer = (LogicalCTEConsumer) p;
- if
(logicalCTEConsumer.getCteId().equals(cteAnchor.getCteId())) {
+ if (logicalCTEConsumer.getCteId().equals(cteId)) {
cteConsumers.add(logicalCTEConsumer);
}
}
return false;
});
- cascadesContext.getCteIdToConsumers().put(cteAnchor.getCteId(),
cteConsumers);
- if (cteConsumers.isEmpty()) {
- return outer;
+ return cteConsumers;
+ }
+
+ /**
+ * The outputs of a CTE producer body are the contract of its consumers,
the consumers map to them by exprId. A
+ * rule which rewrites the producer body may rename them, so compare the
outputs before and after the rewrite to
+ * know which exprIds changed. A column which is not renamed keeps its
exprId, so it needs no mapping, and the
+ * columns which are renamed are the ones whose exprId disappeared: match
them by name, a rule which renames an
+ * output keeps its name. A column which was pruned by the rewrite of the
body, e.g. by ColumnPruning, has no
+ * counterpart in the new outputs any more, so there is nothing to map for
it, see the two CTE tests of
+ * CteLimitPushdownPlanTest and DataTraitTest which prune the columns
their consumers do not read.
+ */
+ @VisibleForTesting
+ static Map<ExprId, ExprId> computeProducerExprIdRemap(List<Slot>
oldOutputs, List<Slot> newOutputs) {
+ Set<ExprId> newExprIds =
newOutputs.stream().map(Slot::getExprId).collect(Collectors.toSet());
+ Set<ExprId> matchedNewExprIds =
Sets.newHashSetWithExpectedSize(newExprIds.size());
+ List<Slot> renamedOldOutputs = new ArrayList<>();
+ for (Slot oldOutput : oldOutputs) {
+ if (newExprIds.contains(oldOutput.getExprId())) {
+ matchedNewExprIds.add(oldOutput.getExprId());
+ } else {
+ renamedOldOutputs.add(oldOutput);
+ }
+ }
+ Map<ExprId, ExprId> remap = new HashMap<>();
+ for (Slot renamedOldOutput : renamedOldOutputs) {
+ List<Slot> candidates = newOutputs.stream()
+ .filter(newOutput ->
!matchedNewExprIds.contains(newOutput.getExprId()))
+ .filter(newOutput ->
newOutput.getName().equals(renamedOldOutput.getName()))
+ .collect(Collectors.toList());
+ if (candidates.size() > 1) {
+ // the name is not a key: several outputs of the producer may
share it. The lineage of a slot
+ // survives a rename (Alias#toSlot copies it), so use it to
find the right candidate.
+ candidates = candidates.stream()
+ .filter(candidate -> sameLineage(renamedOldOutput,
candidate))
Review Comment:
[P2] Preserve CTE mappings for same-named constant outputs
The newly added disambiguation still leaves stale maps for a legal
twice-consumed CTE over nonunique `t` with varying `v`:
```sql
WITH c AS (
SELECT a.k,b.k,a.v,SUM(a.x) s
FROM t a CROSS JOIN t b
WHERE a.k=5 AND b.k=6
GROUP BY a.k,b.k,a.v
)
SELECT l.*,r.* FROM c l CROSS JOIN c r;
```
The producer's aggregate outputs `a.k#A,b.k#B,v,s`. Constant propagation
replaces it with `Project(5 AS k#A2,6 AS k#B2,v,s) -> Aggregate(group=v)`. Both
fresh outputs are named `k`; `Alias.toSlot()` copies lineage only from a direct
slot child, so the literal aliases have none and this filter discards both
candidates. Even retained forwarding lineage cannot distinguish these two
references to the same base-table column. Consequently neither A nor B is
remapped, both consumer maps retain vanished producer IDs, and
`PhysicalPlanTranslator.visitPhysicalCTEConsumer` cannot register the
corresponding consumer SlotRefs. Later passes cannot recover the discarded
correspondence. Unlike the earlier generic CTE-boundary report, this instance
is caused by the new name/lineage matcher. Carry the actual rewrite mapping or
preserve the boundary IDs, and cover same-named outputs in an integration test.
--
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]
