fix(plan): support nested correlated scalar aggregates by iamlinjunhong · Pull Request #26480 · matrixorigin/matrixone

iamlinjunhong · 2026-07-31T03:23:06Z

What type of PR is this?

Which issue(s) this PR fixes:

issue #24997

What this PR does / why we need it:

fix(plan): support nested correlated scalar aggregates

qodo-code-review · 2026-07-31T03:23:12Z

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

XuPeng-SH

Blocking correctness issue in pkg/sql/plan/flatten_subquery.go: the new deepScalarAggregate guard assumes every aggregate except count/starcount is NULL-on-empty. That is not true for approx_count and approx_count_distinct (their executor has emptyNull=false and flushes an empty sketch as 0). pullupThroughAgg turns the correlated inner expression into a GROUP BY key, so an outer key with no matching inner rows loses the aggregate row; the LEFT JOIN then exposes NULL instead of the original 0. For example, if the middle table has a row with ts=0, WHERE x.ts = (SELECT approx_count(y.ts) FROM y WHERE y.id = outer.id) should match when that outer id has no y rows, but this rewrite cannot match it. Please gate this on the aggregate empty-input contract (or conservatively whitelist only NULL-on-empty aggregates) and add a no-matching-key regression test.

iamlinjunhong · 2026-07-31T07:47:08Z

Blocking correctness issue in pkg/sql/plan/flatten_subquery.go: the new deepScalarAggregate guard assumes every aggregate except count/starcount is NULL-on-empty. That is not true for approx_count and approx_count_distinct (their executor has emptyNull=false and flushes an empty sketch as 0). pullupThroughAgg turns the correlated inner expression into a GROUP BY key, so an outer key with no matching inner rows loses the aggregate row; the LEFT JOIN then exposes NULL instead of the original 0. For example, if the middle table has a row with ts=0, WHERE x.ts = (SELECT approx_count(y.ts) FROM y WHERE y.id = outer.id) should match when that outer id has no y rows, but this rewrite cannot match it. Please gate this on the aggregate empty-input contract (or conservatively whitelist only NULL-on-empty aggregates) and add a no-matching-key regression test.

fixed

iamlinjunhong · 2026-08-03T03:26:38Z

Blocking: the new guard validates that the aggregate itself returns NULL on empty and that the consumer outside the deep scalar subquery rejects NULL, but it does not validate the scalar subquery's own result projection.

Using the BVT data added by this PR, this query should return 900 for id=3:
SELECT a.id,
       (SELECT MAX(x.val)
          FROM j_fact x
         WHERE x.ts = (
               SELECT COALESCE(MAX(y.ts), 0)
                 FROM j_fact y
                WHERE y.dim_id = a.id)) AS latest
  FROM j_dim a
 WHERE a.id = 3;
The current head accepts it. The generated plan groups the inner aggregate by y.dim_id, projects COALESCE(MAX(...), 0), and then LEFT JOINs it. For the missing id=3 group there is no right-hand row, so the LEFT JOIN supplies NULL for the entire projected column; COALESCE never executes. The enclosing x.ts = NULL filter drops the ts=0 row and the result becomes NULL instead of 900.

Please either require the complete inner scalar result expression to be proven NULL-propagating from its aggregate(s), or keep this shape on the NYI path, and add this missing-key case to the BVT/unit coverage.

fixed

XuPeng-SH

Deep-reviewed updated exact head 873419c34d7ceab3a25c194c7e1a9503e8314b7c. The previous aggregate/projection/consumer NULL-safety blockers are fixed conservatively, but one structural correctness blocker remains.

P1 — LIMIT/OFFSET become global after per-key decorrelation

The new deepScalarAggregate eligibility checks the aggregate empty-input contract and NULL propagation, but not row-order/row-limit wrappers on the scalar aggregate plan. pullupThroughAgg adds the deep correlation key to GROUP BY, turning the original one-row implicit aggregate into multiple key groups. A LIMIT or OFFSET that originally ran independently inside each correlated scalar invocation then remains on the grouped plan and truncates keys globally.

I reproduced this through SQL on a server built from this exact head using the PR fixture:

shape                       id=1   id=2   id=3
no LIMIT                    220    300    NULL
inner aggregate LIMIT 1     220    NULL   NULL
inner LIMIT 1 OFFSET 1      NULL   300    NULL

LIMIT 1 must be equivalent to no limit because an implicit scalar aggregate already returns exactly one row per invocation. LIMIT 1 OFFSET 1 must return no scalar row for every outer id. The observed result shows that limit/offset are applied once across the decorrelated correlation-key groups.

Please make deep-scalar eligibility validate the complete plan topology from the scalar root to AGG. Keep LIMIT/OFFSET (and any other wrapper whose semantics are per correlated invocation but global after grouping) on the NYI path unless it is explicitly rewritten per key. Add public regressions for both LIMIT 1 equivalence and LIMIT 1 OFFSET 1, with multiple matching and missing outer keys.

Fresh exact-head service build, go list, build, vet, focused new tests, and the full pkg/sql/plan suite pass; the blocker is an uncovered reachable plan shape.

iamlinjunhong · 2026-08-03T06:34:42Z

Deep-reviewed updated exact head 873419c34d7ceab3a25c194c7e1a9503e8314b7c. The previous aggregate/projection/consumer NULL-safety blockers are fixed conservatively, but one structural correctness blocker remains.

P1 — LIMIT/OFFSET become global after per-key decorrelation

The new deepScalarAggregate eligibility checks the aggregate empty-input contract and NULL propagation, but not row-order/row-limit wrappers on the scalar aggregate plan. pullupThroughAgg adds the deep correlation key to GROUP BY, turning the original one-row implicit aggregate into multiple key groups. A LIMIT or OFFSET that originally ran independently inside each correlated scalar invocation then remains on the grouped plan and truncates keys globally.

I reproduced this through SQL on a server built from this exact head using the PR fixture:
shape                       id=1   id=2   id=3
no LIMIT                    220    300    NULL
inner aggregate LIMIT 1     220    NULL   NULL
inner LIMIT 1 OFFSET 1      NULL   300    NULL
LIMIT 1 must be equivalent to no limit because an implicit scalar aggregate already returns exactly one row per invocation. LIMIT 1 OFFSET 1 must return no scalar row for every outer id. The observed result shows that limit/offset are applied once across the decorrelated correlation-key groups.

Please make deep-scalar eligibility validate the complete plan topology from the scalar root to AGG. Keep LIMIT/OFFSET (and any other wrapper whose semantics are per correlated invocation but global after grouping) on the NYI path unless it is explicitly rewritten per key. Add public regressions for both LIMIT 1 equivalence and LIMIT 1 OFFSET 1, with multiple matching and missing outer keys.

Fresh exact-head service build, go list, build, vet, focused new tests, and the full pkg/sql/plan suite pass; the blocker is an uncovered reachable plan shape.

fixed

XuPeng-SH

Deep-reviewed updated exact head 25aca5e219f4df1b5efa69c6983380d118a7ddfa. The previous LIMIT/OFFSET blocker is closed by a conservative topology proof: only PROJECT* -> AGG(aggregateTag) is eligible, LIMIT/OFFSET/RANK are rejected on every accepted node, and SORT/DISTINCT/FILTER/other wrappers remain on the NYI path.

Exact-head public SQL verification covered:

the supported direct aggregate and CAST-projection shapes, both returning 1 -> 220, 2 -> 300, 3 -> NULL;
the earlier missing-group hazards (APPROX_COUNT, inner COALESCE) remaining NYI;
LIMIT 1, LIMIT 1 OFFSET 1, DISTINCT, and SORT wrapper controls all remaining NYI.

go list, package build, vet, focused new tests, the full pkg/sql/plan suite, and an exact-head service build all pass. I found no remaining correctness blocker.

mergify · 2026-08-03T09:35:50Z

Merge Queue Status

✅ Entered queue — 2026-08-03 09:35 UTC · Rule: main · triggered by rule Automatic queue on approval for main
🟠 Checks running · in-place
🚫 Left the queue — 2026-08-03 10:17 UTC · at 1c734a731dae9642d47442c0de9bc84aef83e895

This pull request spent 42 minutes 1 second in the queue, with no time running CI.

Waiting for

All conditions

Reason

Pull request #26480 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.

The author needs to enable "Allow edits from maintainers" on this pull request.

Failing checks:

Hint

You 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.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

Requeue this pull request

iamlinjunhong requested review from XuPeng-SH, aunjgr and heni02 as code owners July 31, 2026 03:23

iamlinjunhong temporarily deployed to ci July 31, 2026 03:23 — with GitHub Actions Inactive

matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jul 31, 2026

iamlinjunhong temporarily deployed to ci July 31, 2026 03:54 — with GitHub Actions Inactive

heni02 approved these changes Jul 31, 2026

View reviewed changes

XuPeng-SH requested changes Jul 31, 2026

View reviewed changes

iamlinjunhong force-pushed the m-24997 branch from a059e74 to 1c48bd5 Compare July 31, 2026 06:41

iamlinjunhong had a problem deploying to ci July 31, 2026 06:41 — with GitHub Actions Failure

iamlinjunhong temporarily deployed to ci July 31, 2026 06:41 — with GitHub Actions Inactive

iamlinjunhong temporarily deployed to ci July 31, 2026 07:14 — with GitHub Actions Inactive

iamlinjunhong requested a review from XuPeng-SH July 31, 2026 07:47

iamlinjunhong force-pushed the m-24997 branch from 1c48bd5 to cf0eba5 Compare July 31, 2026 07:53

iamlinjunhong had a problem deploying to ci July 31, 2026 07:53 — with GitHub Actions Error

iamlinjunhong temporarily deployed to ci July 31, 2026 07:53 — with GitHub Actions Inactive

iamlinjunhong had a problem deploying to ci July 31, 2026 07:53 — with GitHub Actions Error

iamlinjunhong temporarily deployed to ci July 31, 2026 07:53 — with GitHub Actions Inactive

iamlinjunhong had a problem deploying to ci July 31, 2026 07:53 — with GitHub Actions Error

iamlinjunhong requested a review from XuPeng-SH August 3, 2026 03:26

iamlinjunhong temporarily deployed to ci August 3, 2026 03:40 — with GitHub Actions Inactive

iamlinjunhong temporarily deployed to ci August 3, 2026 03:41 — with GitHub Actions Inactive

iamlinjunhong temporarily deployed to ci August 3, 2026 04:12 — with GitHub Actions Inactive

XuPeng-SH requested changes Aug 3, 2026

View reviewed changes

iamlinjunhong added 3 commits August 3, 2026 14:15

fix(plan): support nested correlated scalar aggregates

34f5175

test(plan): restore protocol version after generated expr test

778dc49

fix(plan): reject unsafe deep scalar wrappers

4f03488

iamlinjunhong force-pushed the m-24997 branch from 873419c to 4f03488 Compare August 3, 2026 06:31

iamlinjunhong temporarily deployed to ci August 3, 2026 06:32 — with GitHub Actions Inactive

iamlinjunhong had a problem deploying to ci August 3, 2026 06:32 — with GitHub Actions Error

iamlinjunhong temporarily deployed to ci August 3, 2026 06:32 — with GitHub Actions Inactive

iamlinjunhong had a problem deploying to ci August 3, 2026 06:32 — with GitHub Actions Failure

matrix-meow added size/L Denotes a PR that changes [500,999] lines and removed size/M Denotes a PR that changes [100,499] lines labels Aug 3, 2026

iamlinjunhong requested a review from XuPeng-SH August 3, 2026 06:34

iamlinjunhong had a problem deploying to ci August 3, 2026 07:04 — with GitHub Actions Failure

ci: retry flaky coverage test

25aca5e

XuPeng-SH approved these changes Aug 3, 2026

View reviewed changes

Merge branch 'main' into m-24997

1c734a7

LeftHandCold mentioned this pull request Aug 3, 2026

[Bug]: Planner UT leaks MOProtocolVersion and breaks fuzzy runtime-filter shard #26625

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(plan): support nested correlated scalar aggregates - #26480

fix(plan): support nested correlated scalar aggregates#26480
XuPeng-SH merged 5 commits into
matrixorigin:mainfrom
iamlinjunhong:m-24997

iamlinjunhong commented Jul 31, 2026

Uh oh!

qodo-code-review Bot commented Jul 31, 2026

Uh oh!

XuPeng-SH left a comment

Uh oh!

iamlinjunhong commented Jul 31, 2026

Uh oh!

iamlinjunhong commented Aug 3, 2026

Uh oh!

XuPeng-SH left a comment

Uh oh!

iamlinjunhong commented Aug 3, 2026

P1 — LIMIT/OFFSET become global after per-key decorrelation

Uh oh!

XuPeng-SH left a comment

Uh oh!

mergify Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

iamlinjunhong commented Jul 31, 2026

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

Uh oh!

qodo-code-review Bot commented Jul 31, 2026

Qodo reviews are paused for this user.

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

iamlinjunhong commented Jul 31, 2026

Uh oh!

iamlinjunhong commented Aug 3, 2026

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

P1 — LIMIT/OFFSET become global after per-key decorrelation

Uh oh!

iamlinjunhong commented Aug 3, 2026

P1 — LIMIT/OFFSET become global after per-key decorrelation

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mergify Bot commented Aug 3, 2026 •

edited

Loading