github-actions[bot] commented on code in PR #67682:
URL: https://github.com/apache/doris/pull/67682#discussion_r4032393764
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CTEInline.java:
##########
@@ -86,8 +87,13 @@ public Plan visitLogicalCTEAnchor(LogicalCTEAnchor<? extends
Plan, ? extends Pla
return false;
});
if (mustInlineCTEs.contains(cteAnchor.getCteId())) {
+ LogicalCTEProducer<?> cteProducer = (LogicalCTEProducer<?>)
cteAnchor.left();
+ if (containsNondeterministicFunction(cteProducer)) {
+ throw new AnalysisException("recursive cte must inline all
used ctes, but inline is blocked by"
+ + " none deterministic function");
Review Comment:
[P1] Check volatility, not determinism, before rejecting
`NondeterministicFunctionCollector` classifies every non-`IMMUTABLE` UDF as
nondeterministic, so this also rejects a declared `STABLE` scalar UDF. Doris
deliberately gives `STABLE` UDFs `VolatileIdentity.NON_VOLATILE`; its
volatility tests permit duplicating stable calls, and `ExpressionTrait` says
non-idempotent checks such as `rand()`/`uuid()` should use
`containsVolatileExpression()`. A direct live recursive helper containing a
`STABLE` UDF therefore has no per-call volatility hazard, but this patch newly
turns it into an analysis error. Gate this error on truly volatile expressions
and add a positive stable-UDF recursive case alongside the UUID negative case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CTEInline.java:
##########
@@ -86,8 +87,13 @@ public Plan visitLogicalCTEAnchor(LogicalCTEAnchor<? extends
Plan, ? extends Pla
return false;
});
if (mustInlineCTEs.contains(cteAnchor.getCteId())) {
+ LogicalCTEProducer<?> cteProducer = (LogicalCTEProducer<?>)
cteAnchor.left();
+ if (containsNondeterministicFunction(cteProducer)) {
+ throw new AnalysisException("recursive cte must inline all
used ctes, but inline is blocked by"
Review Comment:
[P1] Restrict this error to live recursive-term dependencies
`mustInlineCTEs` is currently broader than that invariant. `AnalyzeCTE`
supplies the same recursive context while analyzing the non-recursive anchor,
and `BindRelation` does not exclude `isAnalyzingRecursiveCteAnchorChild()`, so
a single anchor-only use such as `u(uuid) -> r` reaches this throw even though
forced inline evaluates `uuid()` once. The check also runs before
`EliminateFilter`, so a recursive `JOIN u ... WHERE FALSE` now fails although
that branch is later proven empty and previously returned only the anchor row.
Both are valid queries newly rejected by this patch. Track whether the
dependency is a live recursive-term use (or move the safety decision after the
relevant simplification) and add positive regressions for the anchor-only and
dead-branch cases.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CTEInline.java:
##########
@@ -86,8 +87,13 @@ public Plan visitLogicalCTEAnchor(LogicalCTEAnchor<? extends
Plan, ? extends Pla
return false;
});
if (mustInlineCTEs.contains(cteAnchor.getCteId())) {
+ LogicalCTEProducer<?> cteProducer = (LogicalCTEProducer<?>)
cteAnchor.left();
+ if (containsNondeterministicFunction(cteProducer)) {
Review Comment:
[P1] Close transitive CTE dependencies before this guard
`mustInlineCTEs` only receives the CTE name bound directly in the recursive
term. For `u AS (SELECT random() x), v AS (SELECT x FROM u), r AS (...
recursive term joins v ...)`, only `v` is marked; its producer contains a leaf
`LogicalCTEConsumer(u)`, so this collector never sees `u`'s `random()`. `v` is
then copied below the recursive union while volatile `u` remains materialized.
That producer fragment is part of the recursive-side reset closure, so it is
rebuilt on later iterations and can produce a different value instead of the
single value required by CTE materialization. Compute the transitive producer
dependency closure before visiting anchors (forcing deterministic dependencies
inline and routing indirect volatile ones through this error), and add a `u ->
v -> r` 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]
