github-actions[bot] commented on code in PR #67682:
URL: https://github.com/apache/doris/pull/67682#discussion_r4037376928
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -195,8 +195,11 @@ private LogicalPlan bindWithCurrentDb(CascadesContext
cascadesContext, UnboundRe
leading.putRelationIdAndTableName(Pair.of(consumer.getRelationId(), tableName));
leading.getRelationIdToScanMap().put(consumer.getRelationId(), consumer);
}
- if (cascadesContext.getRecursiveCteContext().isPresent()) {
- // we are analyzing recursive CTE's recursive child, must
inline all used CTEs
+ if (cascadesContext.getRecursiveCteContext().isPresent()
Review Comment:
[P1] Preserve recursive ownership through child analysis
This seed misses ordinary CTE consumers analyzed inside a recursive term's
scalar/EXISTS subquery (and nested `WITH`): `SubExprAnalyzer` creates that
child `CascadesContext` with a null recursive context. For `u AS (SELECT uuid()
v)` plus a correlated `EXISTS (SELECT 1 FROM u ...)` in `r`'s recursive term,
`BindRelation` never adds `u`; unnesting later leaves `Consumer(u)` under child
1, ordinary `CTEInline` keeps volatile `u` materialized, and the new checker
has no deferred ID. That fragment enters the recursive reset closure,
recreating the unsupported shared-CTE path this PR is meant to prevent.
Preserve enclosing recursive-side ownership across child analysis contexts
(including nesting), or derive the must-inline set structurally from the final
recursive subtree, and add correlated-subquery/nested-`WITH` regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CheckMustInlineVolatileCTE.java:
##########
@@ -0,0 +1,70 @@
+// 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.
+
+package org.apache.doris.nereids.rules.rewrite;
+
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.jobs.JobContext;
+import org.apache.doris.nereids.trees.expressions.CTEId;
+import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.trees.plans.logical.LogicalCTEConsumer;
+import org.apache.doris.nereids.trees.plans.logical.LogicalRecursiveUnion;
+import org.apache.doris.nereids.trees.plans.visitor.CustomRewriter;
+import org.apache.doris.nereids.trees.plans.visitor.DefaultPlanRewriter;
+
+import java.util.Set;
+
+/**
+ * Report recursive ctes which still reference a cte that must be inlined but
contains a volatile
+ * expression, after the dead branches in the recursive child have been
eliminated.
+ *
+ * <p>Every cte referenced by the recursive child of a recursive cte has to be
inlined, because the
+ * recursive child is reset and re-executed on every iteration and therefore
can not read a
+ * materialized cte. A cte containing a volatile expression (rand(), uuid(), a
volatile udf, ...)
+ * can not be inlined either: the volatile expression would be evaluated once
per iteration and once
+ * per reference instead of once for the whole statement. {@link CTEInline}
keeps such ctes
+ * materialized and defers the decision to this rule, which is applied after
the dead branches are
+ * removed, so references which are eliminated as dead code (for example below
a false filter) stay
+ * valid.
+ */
+public class CheckMustInlineVolatileCTE extends DefaultPlanRewriter<Void>
implements CustomRewriter {
+
+ @Override
+ public Plan rewriteRoot(Plan plan, JobContext jobContext) {
+ Set<CTEId> deferredCTEs =
jobContext.getCascadesContext().getStatementContext()
+ .getDeferredInlineVolatileCTEs();
+ if (deferredCTEs.isEmpty()) {
+ return plan;
+ }
+ plan.foreach(node -> {
Review Comment:
[P1] Do not validate an unreachable recursive rerun
This scans child 1 even when the same early rewrite batch has proven the
anchor empty. A plan like `RecursiveUnion(anchor=SELECT 1 WHERE FALSE,
recursive=WorkTable(r) JOIN u(uuid))` retains the union because there is no
empty-relation rule for `LogicalRecursiveUnionAnchor`/`LogicalRecursiveUnion`,
so this throws on `Consumer(u)`. With no anchor rows, BE returns EOS without
starting `_recursive_process` or rebuilding/resetting the recursive fragments,
so there is no repeated materialized-CTE access and the previously valid query
should simply return no rows. This is distinct from the existing
dead-recursive-branch thread: the right child remains syntactically present,
but the unsafe recursive rerun is unreachable. Collapse an empty-anchor
recursive union or skip this check when child 0 is provably empty, and add an
anchor-empty regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CTEInline.java:
##########
@@ -109,8 +112,19 @@ public Plan visitLogicalCTEAnchor(LogicalCTEAnchor<?
extends Plan, ? extends Pla
return false;
});
if (mustInlineCTEs.contains(cteAnchor.getCteId())) {
+ LogicalCTEProducer<?> cteProducer = (LogicalCTEProducer<?>)
cteAnchor.left();
+ if (containsVolatileExpression(cteProducer)) {
Review Comment:
[P1] Scope volatility to the recursive consumer slice
This producer-wide decision happens before consumer-output pruning and
rejects valid recursive uses that need no volatile work. With `u AS (SELECT 1
k, uuid() v)`, a recursive term that uses only `k` is safe to inline and prune.
If no other consumer needs `v`, `RewriteCteChildren` removes UUID but the stale
deferred ID still makes the checker throw; if an outer consumer does need `v`,
the shared producer remains volatile, yet Doris can keep that materialized copy
for the outer consumer and inline a `k`-only copy for the recursive side.
Recomputing volatility only on the globally pruned producer fixes the first
case but not the second. Make forced inlining and volatility validation
consumer/controller scoped after pruning (including cardinality-affecting
expressions), and add both unused-output and mixed-consumer regressions.
--
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]
