Skip to content

fix(group): prevent declared key width overflow - #30999

Open
LeftHandCold wants to merge 2 commits into
matrixorigin:mainfrom
LeftHandCold:codex/group-wide-key-overflow-20261024
Open

LeftHandCold wants to merge 2 commits into
matrixorigin:mainfrom
LeftHandCold:codex/group-wide-key-overflow-20261024

Conversation

@LeftHandCold

Copy link
Copy Markdown
Collaborator

What type of PR is this?

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

Which issue(s) this PR fixes:

Fixes #30997

What this PR does / why we need it:

Grouping by two nullable LONGTEXT columns can fail before reading any input: each declared width plus its NULL marker overflows int32, and the sum wraps to zero. Group then selects the no-key H0 path and dereferences an expression vector before evaluation. Tiny ordinary text values reproduce this; STORED JSON projections expose the same metadata path.

Saturate the width estimate before narrowing or addition, and select H0 from the actual number of grouping keys. Exact small widths, the eight-byte fast path and the existing legacy protocol fence are preserved. This adds no retained data, cache, synchronization, wire format or schema migration.

Validation:

  • Original-code overlay reproduces the panic; fixed tests cover nullable/non-null wide keys, exact eight-byte and global-nullability boundaries, no-key and legacy controls, reuse and mpool cleanup.
  • Real partial Group output merges correctly through MergeGroup under both v77 and v78.
  • Owning Group tests, go vet, configured incremental golangci-lint and formatting/diff checks pass on the reconciled main-based revision.
  • Independent SQL values/metadata and two normal Java BVT comparisons pass for ordinary and STORED LONGTEXT keys (10/10 statements each, zero failures/ignored cases).
  • The complete 20-query AgentLogs workload passes with the isolated fix; this is correctness evidence, not a performance claim.

The causal SQL/BVT and workload binary is based on b787941; the final branch is rebased onto main 156b7cc and revalidates the owning package/static closure. The intervening Group change is downstream spill handling; preparation and the regression case are unchanged. The later main commit 1651eec changes only a Python SDK test. Full CI remains for this draft PR.

@mergify mergify Bot added the kind/bug Something isn't working label Oct 23, 2026
@matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Oct 23, 2026
@XuPeng-SH
XuPeng-SH marked this pull request as ready for review October 24, 2026 01:06
@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 →

@LeftHandCold LeftHandCold left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

代码复核:建议通过,未发现需要 Request Changes 的确定性问题。审查 HEAD:a6eb721152e0aaeda7e398d8f80ad4f7a1513f98。

设计判断:这是局部分组键宽度溢出修复,保留现有 Group/HashMap/MergeGroup 的职责,不需要引入新状态或替代编码路径。

逐项核对:

  • 宽度在转 int32 和相加之前饱和,避免 nullable LONGTEXT/组合键把估算值绕回 0 或小值;H0 由真实键数量决定,而不是依赖可能溢出的宽度。
  • 小固定键的 H8、全局 nullable 标记、v77/v78 兼容选择和 grouping sets 路径仍保留;宽度是编码策略估计,不会按 LONGTEXT 声明大小分配内存。
  • 检查了 partial Group → MergeGroup 的元数据消费。新增测试覆盖无键、8 字节边界、宽键、NULL、旧协议、reset/reuse 和释放;SQL 用少量真实短字符串触发声明宽度问题,不依赖超大数据。

验证边界:已核对当前 HEAD 对应的成功 CI 40015109628,阅读相关实现和新增回归测试,并完成 diff --check。本次没有在本机重跑整套 UT/真实业务负载;PR 中作者的性能/真实数据报告不冒充本次独立实测。当前没有需要关闭的人工 review thread。

当前认证账号也是 PR 作者,GitHub 不允许自审 Approve,因此以 COMMENT 记录“代码审查建议通过”,不是正式 Approval。

This branch was successfully deployed

1 active deployment
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 on two nullable LONGTEXT keys fails after width overflow

3 participants