You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by "yaooqinn (via GitHub)" <gi...@apache.org> on 2026/05/06 12:15:19 UTC

[PR] [SPARK-56738][SQL] Normalize cteIds of orphan CTERelationRef in NormalizeCTEIds [spark]

yaooqinn opened a new pull request, #55708:
URL: https://github.com/apache/spark/pull/55708

### What changes were proposed in this pull request?

Extend `NormalizeCTEIds` so that it normalizes `CTERelationRef.cteId` for "orphan" references whose matching `CTERelationDef` is not present anywhere in the input plan (i.e., the enclosing `WithCTE` lives outside the sub-tree being normalized).

The change is intentionally minimal:
- `apply` first walks the plan to collect the ids of every reachable `CTERelationDef`. References whose `cteId` is in that set are paired with their def and remain handled by the existing `WithCTE` branch (no behaviour change). References whose `cteId` is NOT in that set are treated as orphan and pre-assigned a deterministic id from the same shared counter.
- A new `case ref: CTERelationRef` in `applyInternal` rewrites those orphan refs to the assigned id; refs paired with a def are rewritten as before inside `canonicalizeCTE` and never re-rewritten.

### Why are the changes needed?

`CacheManager.{cacheQuery, lookupCachedData}` run `QueryExecution.normalize` (whose first rule is `NormalizeCTEIds`) so that two parses of the same SQL canonicalize identically and cached entries can be reused. Today `NormalizeCTEIds` only rewrites cteIds it discovers inside a `WithCTE` node. When a caller probes `CacheManager` with a sub-tree that contains a `CTERelationRef` but the corresponding `CTERelationDef` is not in the sub-tree, the per-parse cteId leaks into the canonical form and `sameResult` returns false on a structurally-identical re-parse, breaking cache lookups for that sub-tree. This patch closes that gap so `NormalizeCTEIds` produces a parse-stable canonical form for both wrapped and orphan CTE references.

This fix is scoped to the `NormalizeCTEIds` contract; it does not by itself remove all sources of canonical instability across parses (e.g. operand ordering of commutative expressions or canonicalization of subquery broadcast/dynamic-pruning wrappers), which are tracked separately.

### Does this PR introduce _any_ user-facing change?

No. `NormalizeCTEIds` is an internal plan-normalization rule. Plans without orphan `CTERelationRef` produce byte-identical output to before; plans with orphan refs only see their `cteId` rewritten to a deterministic value.

### How was this patch tested?

New `NormalizeCTEIdsSuite` with two unit tests:
- existing behaviour: `WithCTE`-wrapped plans canonicalize identically across parses (regression guard for the unchanged path).
- new behaviour: orphan `CTERelationRef` sub-trees with structurally identical bodies but different per-parse `cteId` values canonicalize identically and are considered `sameResult` after `NormalizeCTEIds`.

New `CacheManagerSuite` test that parses a `WITH inner_cte AS ..., outer_cte AS (... (SELECT m FROM inner_cte))` SQL twice, extracts `outer_cte`'s body (an orphan `CTERelationRef` sub-tree) and verifies `QueryExecution.normalize` produces the same canonical form / `sameResult` across parses.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: GitHub Copilot CLI, model: Claude Opus 4.7 1M context (claude-opus-4.7-1m-internal)

--
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-56738][SQL] Normalize cteIds of orphan CTERelationRef in NormalizeCTEIds [spark]

Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.

yaooqinn commented on PR #55708:
URL: https://github.com/apache/spark/pull/55708#issuecomment-4387841858

Closing this PR.

After re-reading the change with fresh eyes, the upstream user-visible motivation is too thin: in apache/spark master no code path passes a sub-tree with an orphan `CTERelationRef` (no enclosing `WithCTE`) to `CacheManager.lookupCachedDataInternal`. `Dataset.cache()` / `CACHE TABLE` cache the full analyzed plan with `WithCTE` at top, and `useCachedData`'s `transformDown` walk over a wider query never matches a cached entry on a sub-tree below `WithCTE` (no cached plan is shaped like an orphan `CTERelationRef` body in normal usage).

The real consumer of the fix is downstream CTE-materialization / CTE-inmemory-cache prototyping that explicitly probes `CacheManager` with sub-trees rooted at `CTERelationRef`. That work is not being upstreamed at this point, so this contract fix has no consumer in master.