Repository navigation
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? |
LeftHandCold
left a comment
LeftHandCold
left a comment
There was a problem hiding this comment.
代码复核：建议通过，未发现需要 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。
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: a6eb721152e0aaeda7e398d8f80ad4f7a1513f98
Assessment: approve
Reviewed the complete four-file delta and affected callers, codec consumers, compatibility and cleanup paths. Saturating before narrowing and selecting H0 by key count restores the invariant at the existing owner. Small-key behavior and legacy protocol handling remain intact. No actionable findings.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
Validation:
-
Both focused regression tests passed: 12 subcases covering wide keys, boundaries, legacy controls, reset/cleanup and v77/v78 partial merges. Receipt: 1792809235702452346-3.json.
-
The base implementation overlay reproduced the expected nil-pointer panic during Group preparation. Receipt: 1792809249203440231-3.json.
-
Isolation preflight passed; 192 native inputs matched the retained build and repository provenance verification passed.
-
Reused the supplied exact-head successful UT, SCA and BVT CI snapshot. Source remained clean and the frozen delta matched.
-
Reconciled commands, source fingerprints, exits and logs in /home/xupeng/.local/state/mo-bot/jobs/e2ff2c860acc2f683bd06b24/attempt-1/validation-manifest.json. Initial dependency-fetch failures were recovered before successful test execution.
Limitations:
-
Broader suites were not rerun locally. Author workload logs and the linked issue body were not supplied; unverified workload claims were not used as validation.
-
No performance benchmark was run. Source tracing shows only preparation-time integer checks, with no added per-row work, allocation, I/O or synchronization.
Merge Queue Status
This pull request spent 1 hour 2 minutes 57 seconds in the queue, with no time running CI. ReasonPull request #30999 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.
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. Waiting for any of
All conditions
Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
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:
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.