Skip to content

fix: grouped correlated column binding#25445

Open
LeftHandCold wants to merge 4 commits into
matrixorigin:mainfrom
LeftHandCold:codex/fix-grouped-correlated-subquery
Open

fix: grouped correlated column binding#25445
LeftHandCold wants to merge 4 commits into
matrixorigin:mainfrom
LeftHandCold:codex/fix-grouped-correlated-subquery

Conversation

@LeftHandCold

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes #25352

What this PR does / why we need it:

This fixes ONLY_FULL_GROUP_BY planning for correlated subqueries that reference an outer query column already present in the outer GROUP BY.

  • Correlated references to grouped outer columns now bind to the aggregate/group output instead of the pre-aggregation table scan column.
  • Correlated references to non-grouped outer columns are still rejected under ONLY_FULL_GROUP_BY.
  • Added planner regression tests for both the allowed grouped-column case and the rejected non-grouped-column case.

This PR intentionally does not change the broader invalid-value coercion / IGNORE compatibility behavior discussed in #25364, #25365, and #25366.

Root Cause

When binding a correlated outer column reference from inside a subquery, the planner reused the original table-scan column binding. After the outer query had been grouped, that raw column was no longer available unless it was also part of the aggregate/group output, so valid references to grouped columns could be rejected as non-aggregated columns.

Checks

  • git diff --check -- pkg/sql/plan/base_binder.go pkg/sql/plan/build_test.go
  • GOCACHE=/private/tmp/mo-go-build-cache CGO_CFLAGS="-I$(pwd)/cgo -I$(pwd)/thirdparties/install/include" CGO_LDFLAGS="-L$(pwd)/thirdparties/install/lib -lusearch_c" DYLD_LIBRARY_PATH="$(pwd)/cgo:$(pwd)/thirdparties/install/lib" go test -mod=mod -ldflags="-extldflags '-L$(pwd)/cgo -lmo -L$(pwd)/thirdparties/install/lib -Wl,-rpath,@executable_path/lib'" -count=1 ./pkg/sql/plan

@mergify mergify Bot added the kind/bug Something isn't working label Jul 3, 2026
@LeftHandCold LeftHandCold marked this pull request as ready for review July 3, 2026 09:38
@qodo-code-review

Copy link
Copy Markdown

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 →

@mergify

mergify Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

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

  • Queue this pull request

@XuPeng-SH XuPeng-SH changed the title [codex] fix grouped correlated column binding fix: grouped correlated column binding Jul 3, 2026

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes because the fix is not functionally closed across all correlated-subquery entry points.

Finding:
HAVING still rejects the same valid grouped outer reference that this PR fixes for the SELECT list. A correlated reference to an outer column that is already present in the outer GROUP BY should bind to the outer group output regardless of whether the subquery appears in SELECT, HAVING, or ORDER BY.

Repro on this PR head (c7578373a380c122b7f26b12f55e50e8bbce2ead):

SELECT n_name
FROM nation
GROUP BY n_name
HAVING (SELECT COUNT(*) FROM nation2 n2 WHERE n2.n_name = nation.n_name) > 0;

Current result from local planner test:

SQL syntax error: column "nation.n_name" must appear in the GROUP BY clause or be used in an aggregate function

Root cause: the new grouped-correlated lookup is in baseBindColRef, but HavingBinder.BindColRef has its own ONLY_FULL_GROUP_BY branch and can return the group-by error before the grouped correlated ref is accepted. This leaves the original rule only partially fixed.

Suggested direction:

  • Centralize the “correlated outer ref is allowed iff it resolves to an outer group output” decision so all group-aware binders share it, or update HavingBinder.BindColRef to call the base binding path and allow Expr_Corr whose RelPos == ctx.groupTag while still rejecting ungrouped base-table Expr_Corr under ONLY_FULL_GROUP_BY.
  • Add regression coverage for SELECT-list, HAVING, ORDER BY, table alias, GROUP BY ordinal/alias, and the ungrouped-column rejection case.

Local checks:

  • go test ./pkg/sql/plan -run 'TestOnlyFullGroupBy(Allows|Rejects)CorrelatedSubqueryOn.*Column' passed.
  • Extra temporary closure test passed for table alias, GROUP BY ordinal/alias, ORDER BY, and ungrouped rejection; failed only for the HAVING case above.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on the current head (94315695332bd3a9a8a48280e4d7b273f91abdb3). The previous HAVING gap is fixed, but the current implementation still has one semantic regression around correlated aggregates.

Finding:
HavingBinder.BindColRef now returns the ONLY_FULL_GROUP_BY error for a correlated column inside an aggregate when that correlated ref targets the outer group output. That changes a SELECT-list correlated aggregate with an already-grouped outer column from the existing correlated columns in aggregate function NYI into a false GROUP BY syntax error.

Repro on this PR head:

SELECT n_name,
       (SELECT COUNT(nation.n_name) FROM nation2) AS c
FROM nation
GROUP BY n_name;

Current PR result:

SQL syntax error: column "nation.n_name" must appear in the GROUP BY clause or be used in an aggregate function

On origin/main, the SELECT-list variant returns the existing unsupported-path error:

correlated columns in aggregate function is not yet implemented

Why this matters: nation.n_name is already grouped in the outer query, so this is not an ONLY_FULL_GROUP_BY violation. It is either a correlated-aggregate feature to implement deliberately, or it should keep the current NYI behavior. The new TestOnlyFullGroupByPreservesCorrelatedAggregateNYI covers ungrouped/no-group variants, but it misses the grouped variant. The third case in TestOnlyFullGroupByRejectsCorrelatedSubqueryOnUngroupedColumn also appears to lock in the wrong rule: SUM(nation.n_regionkey) uses an outer column that is grouped by the outer query, so expecting a group-by error there is not first-principles correct.

Suggested direction:

  • Keep aggregate-internal correlated refs under one rule for now: Expr_Corr inside aggregate should stay NYI until correlated aggregate semantics are intentionally implemented.
  • Add regressions for grouped outer refs inside subquery aggregates in both SELECT-list and HAVING paths.
  • Keep the already-added positive coverage for simple grouped correlated refs in SELECT/HAVING/ORDER BY.

Local checks performed:

  • git diff --check -- pkg/sql/plan/base_binder.go pkg/sql/plan/having_binder.go pkg/sql/plan/build_test.go test/distributed/cases/subquery/subquery-with-any.result
  • go test ./pkg/sql/plan -run 'TestOnlyFullGroupBy(Allows|Rejects|Preserves)'
  • full go test ./pkg/sql/plan
  • temporary closure tests for expression GROUP BY rejection, alias/base-table scoping, and the grouped correlated aggregate regression above

@aunjgr aunjgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix. Redirecting correlated outer column references bound to GROUP BY columns from the raw table-scan column to the aggregate group output is the correct approach. Test coverage is thorough across grouped-allowed, non-grouped-rejected, and correlated-aggregate-NYI categories.

…grouped-correlated-subquery

# Conflicts:
#	pkg/sql/plan/having_binder.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: GROUP BY query rejects correlated subquery referencing grouped column

6 participants