Skip to content

fix only full group by - #26635

Open
daviszhen wants to merge 15 commits into
matrixorigin:mainfrom
daviszhen:0803-only-full-group-by
Open

fix only full group by#26635
daviszhen wants to merge 15 commits into
matrixorigin:mainfrom
daviszhen:0803-only-full-group-by

Conversation

@daviszhen

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:

issue #25327

What this PR does / why we need it:

  • 未开启 ONLY_FULL_GROUP_BY:保持原有宽松行为。

  • 开启且无 MATRIXONE_NATIVE:

    • 允许 WHERE 限制为单值的非分组列。
    • 允许依赖完整主键的非分组列。
  • 同时包含 MATRIXONE_NATIVE:保持 MatrixOne 原生严格校验。

  • 支持 SELECT、HAVING、ORDER BY,并处理组合主键、ROLLUP/CUBE、ENUM/SET、带点号列名等边界。

  • SQL mode 改变时清理普通计划缓存,并重建模式不匹配的 PREPARE 计划。

  • 新增独立 BVT SQL/result 文件及 planner、frontend 回归测试。

# Conflicts:
#	pkg/frontend/computation_wrapper.go
#	pkg/frontend/mysql_cmd_executor.go
#	pkg/sql/plan/query_builder.go
#	pkg/sql/plan/types.go
@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 →

@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.

Request changes: keep the newly allowed columns valid across the aggregate/window stage boundary.

A valid single-value exception fails to build as soon as that column is consumed by a window function:

SELECT deptno,
       FIRST_VALUE(job) OVER (),
       SUM(sal)
FROM constraint_test.emp
WHERE job = 'clerk'
GROUP BY deptno;

Under ONLY_FULL_GROUP_BY, job is statement-wide single-valued, so this is the same deterministic case this PR is intended to allow. On exact head dbea0aaa827d, a planner regression expecting BuildPlan to succeed instead fails with Column remapping failed: cannot find column reference. The failing aggregate contains any_value(first_value(emp.job) over (...)): wrapBareColRefsInAnyValue walks every project after validation, sees the window-tag project as a bare column, and creates a pre-window aggregate that references a post-window result. Meanwhile, the raw window input also has to be materialized from the aggregate output.

Please rewrite allowed raw window arguments/partition/order expressions to group/aggregate outputs, while leaving the window-result tag above the aggregate stage. Add regressions for both exception shapes (WHERE-single-valued and PK-dependent) in window arguments/order keys. The PR's focused OFGB and sql-mode/prepared-cache tests pass, so this stage boundary is currently uncovered.

@daviszhen

Copy link
Copy Markdown
Contributor Author

Request changes: keep the newly allowed columns valid across the aggregate/window stage boundary.

A valid single-value exception fails to build as soon as that column is consumed by a window function:

SELECT deptno,
       FIRST_VALUE(job) OVER (),
       SUM(sal)
FROM constraint_test.emp
WHERE job = 'clerk'
GROUP BY deptno;

Under ONLY_FULL_GROUP_BY, job is statement-wide single-valued, so this is the same deterministic case this PR is intended to allow. On exact head dbea0aaa827d, a planner regression expecting BuildPlan to succeed instead fails with Column remapping failed: cannot find column reference. The failing aggregate contains any_value(first_value(emp.job) over (...)): wrapBareColRefsInAnyValue walks every project after validation, sees the window-tag project as a bare column, and creates a pre-window aggregate that references a post-window result. Meanwhile, the raw window input also has to be materialized from the aggregate output.

Please rewrite allowed raw window arguments/partition/order expressions to group/aggregate outputs, while leaving the window-result tag above the aggregate stage. Add regressions for both exception shapes (WHERE-single-valued and PK-dependent) in window arguments/order keys. The PR's focused OFGB and sql-mode/prepared-cache tests pass, so this stage boundary is currently uncovered.

  • 防止窗口结果 windowTag 被错误包进聚合 any_value(),保持在 AGG 之后的 WINDOW/PROJECT 阶段。
  • 将窗口函数参数、PARTITION BY、ORDER BY 中允许的裸列改写为聚合输出,避免 AGG 引用窗口阶段列。
  • 新增规划器 UT:WHERE 单值约束、原始 OVER ()、主键函数依赖三种场景。
  • 新增对应 OFGB BVT 及结果文件,覆盖窗口参数、分区键与排序键。

# Conflicts:
#	pkg/sql/plan/base_binder.go

@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.

Re-reviewed exact head 7de4fc4. The previously reported aggregate/window stage-boundary failure is fixed: windowTag results remain above AGG, while allowed raw window arguments, PARTITION BY expressions, and ORDER BY expressions are materialized through aggregate outputs. I verified the focused planner/frontend regressions plus additional edge cases for window-only ORDER BY, isolated partition/order keys, nested scalar wrappers, mixed safe/unsafe columns, reverse equality, OR rejection, and ROLLUP. Build/vet and current CI are green. No blocking findings.

@mergify

mergify Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-08-04 14:15 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • Checks failed · in-place
  • 🚫 Left the queue2026-08-04 14:31 UTC · at e2a00c60a05777173c9cc45e5de6c7831e7af0f9

This pull request spent 16 minutes 21 seconds in the queue, with no time running CI.

Waiting for
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-skipped = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-success = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone CI / UT Test on Ubuntu/x86
    • check-skipped = Matrixone CI / UT Test on Ubuntu/x86
    • check-success = Matrixone CI / UT Test on Ubuntu/x86
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone CI / SCA Test on Linux/arm64
    • check-skipped = Matrixone CI / SCA Test on Linux/arm64
    • check-success = Matrixone CI / SCA Test on Linux/arm64
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
    • check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
    • check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone Utils CI / Coverage
    • check-skipped = Matrixone Utils CI / Coverage
    • check-success = Matrixone Utils CI / Coverage
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
    • check-skipped = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
    • check-success = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
All conditions
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-skipped = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-success = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone CI / UT Test on Ubuntu/x86
    • check-skipped = Matrixone CI / UT Test on Ubuntu/x86
    • check-success = Matrixone CI / UT Test on Ubuntu/x86
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone CI / SCA Test on Linux/arm64
    • check-skipped = Matrixone CI / SCA Test on Linux/arm64
    • check-success = Matrixone CI / SCA Test on Linux/arm64
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
    • check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
    • check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone Utils CI / Coverage
    • check-skipped = Matrixone Utils CI / Coverage
    • check-success = Matrixone Utils CI / Coverage
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
    • check-skipped = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
    • check-success = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
  • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
  • github-review-approved [🛡 GitHub branch protection]

Reason

The merge conditions cannot be satisfied due to failing checks

Failing checks:

Hint

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dequeued kind/bug Something isn't working size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants