fix(plan): support Prisma correlation through transparent derived tables - #26672
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking: keep deeper ancestor correlations fail-closed.
analyzeTransparentDerivedFilter accepts every positive CorrColRef.Depth unless a depth-1 tag is owned by the current FROM context. That also accepts a transparent derived filter whose correlation crosses a real intermediate binding scope. After the unconditional depth decrement, the existing deep-correlation guard is bypassed and optimization receives a dangling outer column.
Minimal public planner witness:
SELECT n1.N_NATIONKEY
FROM NATION n1
WHERE EXISTS (
SELECT 1 FROM NATION n2
WHERE n2.N_NATIONKEY = n1.N_NATIONKEY
AND EXISTS (
SELECT 1 FROM (
SELECT n3.N_NATIONKEY FROM NATION n3
WHERE n3.N_NATIONKEY = n1.N_NATIONKEY
) d
)
);On exact head f1aee5c9905366e4c71ad05042faed1a7b30cbbe, BuildPlan reaches optimizer validation and fails with Column remapping failed: cannot find column reference, missing RelPos=6 in a FILTER. The immediate-ancestor control (n3.N_REGIONKEY = n2.N_REGIONKEY) succeeds. The same SQL on base 21908973304c4117ee2f19670d1d8acd218cd0fb remains at correlated subquery in FROM clause is not yet implemented, and the equivalent direct deep-correlation control remains at the existing deep correlated predicate containing inner columns ... is not yet implemented boundary. A mixed immediate/grandparent predicate fails the same way.
Please either prove and implement the downstream deep-correlation/remapping closure, or atomically reject correlations that cross a non-transparent ancestor scope before mutating depths/context. Add immediate-parent, grandparent-only, and mixed-owner controls so the intended Prisma chain stays supported without broadening this unsafe state.
The submitted package tests, full ./pkg/sql/plan/... test suite, build, vet, and diff check otherwise pass.
|
Fixed on exact head The normalizer now accepts a correlation only when its binding tag belongs to the nearest non-empty ancestor scope. This preserves the intended immediate-parent Prisma chain while rejecting grandparent-only and mixed immediate/grandparent correlations before any Added public planner controls for immediate-parent success plus grandparent-only and mixed-owner NYI rejection, and a synthetic depth-3 atomicity assertion. Focused tests, the full |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head caa830c. The previous non-empty intermediate-scope grandparent and mixed-owner witnesses are now rejected atomically, and the intended immediate-parent Prisma shape still passes. The same scope-ownership defect remains when the intervening query block has no FROM bindings.
P1 — Empty intermediate query scopes are incorrectly treated as binderless transparent wrappers
transparentDerivedCorrelationTargetsNearestAncestor walks ancestors and stops only when len(ancestor.bindingByTag) > 0. An empty SELECT query block has an empty bindingByTag, but it is still a real correlation/decorrelation level. Skipping it accepts a grandparent correlation, decrements CorrColRef.Depth, and again hands the optimizer a dangling outer column.
Public witness on this exact head:
SELECT n1.N_NATIONKEY
FROM NATION n1
WHERE EXISTS (
SELECT 1
WHERE EXISTS (
SELECT 1 FROM (
SELECT n3.N_NATIONKEY FROM NATION n3
WHERE n3.N_NATIONKEY = n1.N_NATIONKEY
) d
)
);
Head result: BuildPlan reaches optimizer validation and fails with Column remapping failed: cannot find column reference, missing RelPos=6 in a FILTER. Exact base b4a6da6 rejects the same SQL at the safe correlated subquery in FROM clause is not yet implemented boundary. The immediate-parent n2 control added by this PR succeeds.
Please distinguish a genuinely binderless derived wrapper from a binder-backed query scope that merely has no table bindings, or otherwise fail closed before changing depth/context. Add empty-intermediate and mixed empty/non-empty ancestor controls so every query-scope shape is covered, not only scopes with bindingByTag entries.
Unhappy-path audit: Q1 correlation ownership crosses a real empty scope and is transferred to the wrong ancestor; Q2 the failure surfaces later as an optimizer remap error after normalization rather than at the supported NYI boundary; Q3 no unbounded-growth or resource-lifecycle risk was found. Existing focused positive/negative planner tests, their race run, plan vet, diff check, and CI pass; this public counterexample is missing.
|
Fixed on exact head The ancestor-ownership check now treats every context with an installed binder as a real correlation level, even when that query block has no Coverage added:
Validation on the exact head:
Fresh exact-head CI is now pending. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-review of 71996f7: the empty-scope blocker is resolved.\n\nThe new boundary matches column binding semantics: build-time contexts without a binder remain transparent, while every binder-backed query block is treated as a real correlation level even when bindingByTag is empty. The ownership check runs during analysis, before depth/context mutation, so rejected deep or mixed-owner shapes remain atomic and fail at the existing NYI boundary rather than reaching optimizer remapping.\n\nFocused local validation passed for:\n- intended Prisma/nested transparent-derived success shapes\n- same-scope lateral, unsupported-plan, grandparent, mixed-owner, empty/non-empty ordering rejections\n- typed depth/context atomicity\n- prepared numeric assignment propagation\n- additional double-empty-scope rejection and multi-wrapper nearest-owner success counterexamples\n\nNo blocking correctness issue found.
Merge Queue Status
This pull request spent 1 hour 17 minutes 1 second in the queue, including 47 minutes 59 seconds running CI. Required conditions to merge
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24737
What this PR does / why we need it:
Prisma 7.9.1 loads MySQL to-many relations with a correlated scalar aggregate over several transparent derived tables. MatrixOne rejected that valid shape with
20102: correlated subquery in FROM clause is not yet implemented.This PR:
PROJECT/FILTERchains ending in oneTABLE_SCAN;CorrColRefdepth or parent correlation state;FROM, so this does not implicitly addLATERALor skip a real correlation level;LEFT JOINdecorrelation and the empty-input fallback merged by fix(plan): preserve empty correlated aggregate projections #26510;Relation-local
orderBy/take/skipremains out of scope and requires later per-parent Top-N work.Base/head evidence
d6800b98855b871459c3c4bad0ce73aa7321610f21908973304c4117ee2f19670d1d8acd218cd0fb71996f78f47dd697efd850ba54b2e1af40249f1d1, 46/49; the raw Prisma SQL, local-filter variant, and derivedCOUNTall returnedcorrelated subquery in FROM clause is not yet implemented.0, 54/54.Validation
TestTransparentCorrelatedDerivedTableandTestPreparedNumericAggregateReachesCorrelatedDerivedTableCGo planner tests..agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=120s ./pkg/sql/plan/....GOWORK=off go vet -mod=readonly ./pkg/sql/plan/....make buildon the exact head.scalar_correlated_projection, exit0, 54/54, including empty-intermediate and both mixed empty/non-empty ancestor orderings.git diff --check.mo-self-reviewwith zero unresolved blockers.PASS review=PASS validation=PENDING exact-head=71996f78f47dd697efd850ba54b2e1af40249f1d diff-hash=1c2010bc41e371f7e0e9680cf97334c46016527b145f7ca41f26072f502b3287.@prisma/adapter-mariadb7.9.1 fixture; the raw generated SQL executed unchanged andrelationLoadStrategy: joindecoded the same 0/1/many and nullable-field objects asquery(DEEP_EQUAL=true).upstream/main4b01f8b4bc36e461a48f09d191df4d8637ee3591, followed by the CR repair focused ancestor-scope planner matrix, full./pkg/sql/plan/...CGo tests,go build,go vet,make build,git diff --check,mo-self-review, and exact-head preflight (PASS review=PASS validation=PENDING).upstream/main6091b418fd529badca4e1855843bd38804de396fkeeps the reported empty-intermediate SQL on the safecorrelated subquery in FROM clause is not yet implementedboundary.Test requirements
The temporary Node/npm fixture is not committed, and this PR does not modify
mo-auto-test.