sql: fix decorrelation of correlated CTEs referenced from nested correlated scopes#37506
Conversation
74c72d4 to
dd9692a
Compare
CTE LocalIds were allocated as `self.ctes.len() + offset`, where `self.ctes` is the map of CTE names currently in scope. A shadowing CTE does not grow that map, so a CTE in a nested scope could get the same LocalId as the shadowing CTE. Later phases key CTEs by LocalId (`qcx.ctes` during planning, `CteMap` during lowering), so a reference to the shadowed name silently bound to the wrong CTE, returning wrong results. A colliding Let inside a correlated subquery could also trip the id-uniqueness assertion in lowering's `branch()`. Allocate ids from a monotonic per-statement counter instead. For queries that were already correct, only the id labels in EXPLAIN RAW PLAN output change (MIR ids are allocated independently during lowering), hence the re-blessed plans in cte_lowering.slt and raw_plan_as_text.slt. Regression tests in cte.slt, expected results verified against PostgreSQL 16. Fixes SQL-355. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dd9692a to
33c302a
Compare
|
@mgree, this is ready for review. |
mgree
left a comment
There was a problem hiding this comment.
Looks good, though it's hard for me to follow every detail of the column tracking. Not sure my suggestion is an improvement, so feel free to ignore it.
| if position < max_cte_outer_arity { | ||
| prefix.push((position, col)); | ||
| } else { | ||
| rest.push((position, col)); | ||
| } | ||
| } | ||
| prefix.sort_unstable_by_key(|(position, _)| *position); | ||
| // The CTE discovery above inserts the columns at all positions | ||
| // `0..cte_outer_arity` of each referenced CTE, and `col_map` is a | ||
| // bijection onto `0..col_map.len()`, so the prefix consists of | ||
| // exactly the positions `0..max_cte_outer_arity`. If it doesn't, the | ||
| // `Get` arm's prefix reconciliation would join on the wrong columns and | ||
| // silently produce wrong results, so fail the query instead. There is | ||
| // no safe fallback: the unpartitioned key order is the bug this is | ||
| // fixing. | ||
| if !prefix | ||
| .iter() | ||
| .map(|(position, _)| *position) | ||
| .eq(0..max_cte_outer_arity) |
There was a problem hiding this comment.
If we know the exact length, could we instead write into the vec by index rather than sorting after? Might be clearer. (I'm definitely having some trouble tracking all of the various column bits and bobs!)
There was a problem hiding this comment.
Thanks! I went back and forth here. Writing into a fixed-size vec by index does drop the sort, but it trades it for Option slots plus separate duplicate- and gap-checks, so I think it's roughly a wash on complexity rather than a clear win — and the genuinely hard-to-follow part is the level/position/ColumnRef juggling, which that rewrite doesn't really change. So I kept the sort but added comments around it: naming what position is, spelling out that prefix-first-then-sorted is what makes key[i] == i (the property the Get arm relies on), and clarifying the -1 level compensation. Hopefully that makes it easier to follow — happy to revisit if you'd still prefer the index version.
…elated scopes When a correlated CTE is referenced from a nested correlated scope, HIR-to-MIR lowering could decorrelate it with the wrong correlation, producing wrong results (SQL-349). Two interacting causes in `branch()`: 1. The walk that collects the outer columns of referenced CTEs used the deprecated `visit`, which does not descend into scalar subqueries. A CTE referenced only from inside a scalar subquery was therefore missed, so its outer columns were dropped from the branch key. 2. The branch key followed `BTreeSet<ColumnRef>` order (level ascending). The `Get` arm reconciles a CTE reference with the relation the CTE was applied to by equating the first `cte_outer_arity` columns of both sides, which assumes the CTE's outer columns sit at the key's prefix in order. A level-1 column sorting before the CTE's outer column broke that assumption. Fix both: discover CTE references with `visit_post` (descends into subqueries at any depth) and partition the branch key so the CTE outer columns form its prefix, ordered by position. Referenced CTEs' outer relations are nested prefixes of one another, so a single prefix covers them all. Gate both changes behind the `enable_fixed_correlated_cte_lowering` optimizer feature flag (default on). With the flag off, lowering is byte-identical to before, giving a rollback lever for upgrade safety: the plan change only lands for objects that reference a correlated CTE from a correlated scope. Regression tests and a flag-off EXPLAIN documenting the old plan in cte_lowering.slt, expected results verified against PostgreSQL 16. Fixes SQL-349. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
33c302a to
9e53aac
Compare
Stacked on top of #37505, the first commit should be reviewed there.
Motivation
Fixes SQL-349.
When a correlated CTE is referenced from a nested correlated scope, HIR-to-MIR lowering could decorrelate it with the wrong correlation, producing wrong results. Example (results verified against PostgreSQL):
Note
Stacked on #37505 and should be merged after it. That PR makes CTE
LocalIds unique, which this PR's deeper CTE-discovery walk relies on (theLetid-uniqueness assertion inbranch()would otherwise fire). Until #37505 merges, this PR's diff includes its commit (sql: allocate globally unique LocalIds for CTEs); review only the second commit here. I'll rebase ontomainonce #37505 lands.Description
Two interacting causes in lowering's
branch():visit, which does not descend into scalar subqueries. A CTE referenced only from inside a scalar subquery was therefore missed, so its outer columns were dropped from the branch key.BTreeSet<ColumnRef>order (level ascending). TheGetarm reconciles a CTE reference with the relation the CTE was applied to by equating the firstcte_outer_aritycolumns of both sides, which assumes the CTE's outer columns sit at the key's prefix, in order. A level-1 column sorting before the CTE's outer column broke that assumption.The fix addresses both: discover CTE references with
visit_post(descends into subqueries at any depth) and partition the branch key so the CTE outer columns form its prefix, ordered by position. Referenced CTEs' outer relations are nested prefixes of one another (nested scopes only ever append columns), so a single prefix covers them all.Both changes are gated behind the new
enable_fixed_correlated_cte_loweringoptimizer feature flag, default on. With the flag off, lowering is byte-identical to before, which gives a rollback lever for upgrade safety: the plan change only lands for objects that reference a correlated CTE from a correlated scope, and flipping the flag off and restarting re-optimizes back to the old plans.Follow-up outside this PR: the
enable_fixed_correlated_cte_loweringflag must be created in LaunchDarkly to be controllable in production. It is temporarily added toKNOWN_MISSING_FROM_LDso the nightly LD-consistency check does not fail meanwhile.Verification
Regression tests added to
test/sqllogictest/cte_lowering.slt, along with a flag-offEXPLAIN DECORRELATED PLAN WITH(enable fixed correlated cte lowering = false)that documents the old (broken) plan next to the fixed one. Expected query results verified against PostgreSQL 16. Ran the CTE, subquery, and decorrelated/optimized/raw plan-as-text sqllogictest files with no fallout beyond the intendedcte_lowering.sltre-bless.Nightly subset run: https://buildkite.com/materialize/nightly/builds/17069