fix(plan): preserve empty correlated aggregate projections - #26510
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? |
matrix-meow
added
size/L
…mpty-correlated-agg
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.
Deep-reviewed exact head fcefaa8a6a5aff7c1d8b0150b7aeefa4904c92da. Requesting changes for one remaining same-shape correctness gap.
P1 — Empty-input reconstruction is incomplete and aggregate-name-specific
prepareCorrelatedScalarAggregatePostJoinProjection hard-codes only sum/avg/min/max/json_arrayagg/count/starcount. For every other aggregate it falls back to the legacy grouped-correlated plan. A missing correlation key then has no right-hand group, so the LEFT JOIN produces NULL even when the aggregate contract requires a non-NULL empty value.
I reproduced this through SQL on a server built from this exact head. The independent empty-input control returned:
BIT_AND(empty) = 18446744073709551615
BIT_OR(empty) = 0
BIT_XOR(empty) = 0
APPROX_COUNT_DISTINCT(empty) = 0
For the equivalent correlated scalar subqueries whose outer key has no matching child row, this head returned NULL for all four. COUNT(empty)=0 and SUM(empty)=NULL were correct controls. Thus the rewrite still violates the core invariant: an implicit scalar aggregate over a correlated empty input must produce the same scalar value as that aggregate over the equivalent uncorrelated empty input.
Please drive post-join reconstruction from a complete canonical aggregate empty-input contract rather than a local name whitelist (or conservatively keep unsupported shapes from producing a wrong plan). Apply it to every aggregate referenced by the final scalar projection, including mixed expressions. Add a public metamorphic regression comparing correlated missing-key results with equivalent uncorrelated empty-input results across both NULL-on-empty and neutral-on-empty aggregate families; include BIT_AND/OR/XOR and APPROX_COUNT_DISTINCT.
Fresh go list, build, vet, focused new regressions, full pkg/sql/plan tests, and the exact-head service build pass; the blocker is semantic coverage, not test instability.
|
Addressed the new empty-input aggregate finding on exact head
Validation on this exact head:
The prior CI cancellations were superseded by run |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed exact head eb983b2a748c. The previous blocker is closed: empty-input reconstruction is now driven by a complete current aggregate-ID contract in aggexec, with NULL/zero/all-bits-set/unsupported outcomes, and is applied to both direct AGG and PROJECT→AGG shapes. BIT_AND/OR/XOR, APPROX_COUNT_DISTINCT, mixed expressions, typed uint64/binary identities, and conservative unsupported paths are covered; the public regression compares correlated missing-key results with equivalent uncorrelated empty inputs.
Fresh exact-head evidence: full pkg/sql/colexec/aggexec and pkg/sql/plan tests pass, controlled build/vet pass, diff check passes, and GitHub CI is green. No remaining blocker found.
Merge Queue Status
This pull request spent 43 minutes 39 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #26510 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
Failing checks:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #25959
What this PR does / why we need it:
Correlated scalar aggregates are decorrelated by grouping the inner rows on the pulled-up correlation key and joining the result back with a
LEFT JOIN. When an outer key has no inner group, an expression evaluated below that join never runs, so examples such asCOALESCE(SUM(v), 0),COUNT(*) + 1,CASE, and the JSON empty-array fallback produce the wrong value.For the supported direct
AGGandPROJECT -> AGGshapes, this change:aggexecand restores typedNULL, zero, or all-bits-set results after null extension;COALESCE,IFNULL,CASE, arithmetic, or a multi-aggregate final expression above the join;HAVING, row-order/row-limit wrappers, deep correlation, and non-aggregate inner-column references without partial plan mutation.The compatibility oracle for this PR is MySQL 8.0.45, as recorded in #25959. Prisma
relationLoadStrategy: joincompatibility remains follow-up #24737 scope.This PR does not modify executor execution behavior, the parser, protobuf, public API, persisted formats,
LATERAL,Node_APPLY,mo-auto-test, or Node/npm dependencies.Validation
d29b4758f88f0894feeca15f31412446c17cb9d3eb983b2a748c8069845b4924680d89df591f3de8test/distributed/cases/subquery/scalar_correlated_projection.sql1,31/39, 8 target semantic failures0,40/40BIT_AND,BIT_OR,BIT_XOR,APPROX_COUNT_DISTINCT, andSUM.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=300s ./pkg/sql/colexec/aggexec ./pkg/sql/plan: PASSExpr_Corrtests: PASSGOWORK=off go build -mod=readonly ./pkg/sql/plan/... ./pkg/sql/colexec/aggexec/...: PASSGOWORK=off go vet ./pkg/sql/plan/... ./pkg/sql/colexec/aggexec/...: PASSmake build: PASSgit diff --check: PASSmo-self-review: PASS, no unresolved findingsmo-pr-preflight-review:PASS review=PASS validation=PENDING, diff hash20cdcad04d6ecfa637ca9c41ab10f0b6296dfc2250e423c2251685b3f4f57ca9BVT
BVT: required
The canonical
subquery/scalar_correlated_projectioncase covers empty/present/all-NULL inner inputs, repeated parent keys, SUM/AVG/MIN/MAX, raw SUM, COUNT variants, BIT_AND/OR/XOR, APPROX_COUNT_DISTINCT, arithmetic, multiple aggregates, CASE, JSON fallback, a forwarding CTE, the issue-authentic CTE-internal correlation, and explicit GROUP BY/HAVING controls.QA
QA required: yes
After merge, verify the original #25959 query and the JSON empty-array fallback on an identified MatrixOne build/environment. Keep #25959 open until the tested version/environment and terminal PASS are recorded.
This is the prerequisite for #24737. Real Prisma 5.22/current
relationLoadStrategy: joinQA remains PENDING and belongs to the follow-up PR/issue lifecycle; it is not claimed by this PR.