add within group - #26781
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? |
…en/matrixone into 0807-add-within-group-by
iamlinjunhong
left a comment
There was a problem hiding this comment.
Requesting changes for four P1 blockers on head 6fc7078b7c01df729b9a08d24a25220de92cda2f:
- The new aggregate IDs can be sent to previous-version CNs during a rolling upgrade without a capability gate.
- Exact percentile state is unbounded for H0 and a single hot group, so normal large inputs can OOM despite
SpillMem. - Making
WITHINreserved breaks SQL and schemas that previously used it as an unquoted identifier. PERCENTILE_CONTdeclares an invalid/insufficient decimal result type for valid maximum-precision inputs and can overflow even at p=0.
I reviewed the full diff from merge-base a4b0ce286d182c24efd5a349620ae37016262301, including parser → binder → plan → compile → remote aggregate → merge/spill/flush/free. All 26 GitHub checks are complete with no failing conclusion. The added parser tests pass locally; a base/head parser probe also confirms the WITHIN compatibility regression. No P2/P3-only items are being used to block this review.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head 6fc7078 after the successful CI rollup.
[P1] pkg/sql/colexec/aggexec/ordered_percentile.go:49: PERCENTILE_CONT always declares decimal128 scale input scale plus one. A valid DECIMAL(38,38) input therefore becomes the invalid DECIMAL(38,39), and flush multiplies the 38-scale unscaled value by 10; values such as 0.9 overflow even for percentile 0, which should simply return the input. Cap or otherwise choose a representable result precision/scale and test the maximum-scale boundary.
[P2] pkg/sql/parsers/dialect/mysql/mysql_sql.y:12524: GROUP_CONCAT accepts both its in-parentheses ORDER BY and WITHIN GROUP ORDER BY, then silently discards the first list in favor of the second. Reject the ambiguous double-order form instead of changing its meaning silently, and add parser coverage.
|
|
There was a problem hiding this comment.
Requesting changes on exact head 15d9b16e832431baa3cdedc892813aab2ded3892 for two P1 blockers:
- Maximum-width DECIMAL inputs can still make
PERCENTILE_CONTreturn a discrete rounded value instead of the required continuous interpolation. - The contextual
WITHINlexer still breaks existing identifier use when the next token isGROUP.
I read the previous review, all review threads, and the PR conversation. The author replies explicitly address the previous findings, and the protocol gate, bounded spill path, basic identifier cases, and double-ORDER rejection are present; the two blockers below remain on the current head.
Review scope: full diff from merge-base 3c7392a0f86b23712da71f31d7a9198a2c8591a9, including parser → binder → plan/type inference → compile/remote protocol → aggregate merge/spill/flush/free and tests. All 26 GitHub checks are complete with no failing conclusion. Local targeted tests could not start in the isolated worktree because the required cgo/libmo.dylib artifact is absent; that is a test prerequisite failure, not test evidence. Both blockers below are directly provable from the current code paths.
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #25144
#25110
What this PR does / why we need it:
新增 SQL WITHIN GROUP (ORDER BY ...) 语法及 AST、Binder、Explain 支持。
GROUP_CONCAT 支持两种排序写法:
新增 PERCENTILE_CONT 和 PERCENTILE_DISC 标量聚合,支持分组、ASC/DESC、NULL、数值类型及分布式合并。
百分位排序改为复用查询 ORDER BY 的 mosort.SortByVectors 实现。
增加聚合 spill、配置编码、资源释放和 DISTINCT group-concat spill 安全保护。
补充 parser、plan、executor、边界测试及 BVT 结果文件。
当前仍是 MVP:不支持百分位窗口函数、参数化 p、非数值排序列和 decimal256。