fix(function): optimize serial_extract to avoid full tuple deserialization - #24246
Merged
mergify[bot] merged 7 commits intoApr 30, 2026
Merged
Conversation
…ation (matrixorigin#24241) When serial_extract is called with a constant index (the common case in GROUP BY on composite secondary index), use UnpackNthElement to decode only up to the target element instead of deserializing the entire tuple. This reduces CPU overhead ~6x for DISTINCT queries on leading columns of composite secondary indexes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jiangxinmeng1
requested review from
XuPeng-SH,
aunjgr and
ouyuanning
as code owners
April 29, 2026 06:07
Secondary index tables were missing Primary: true on the __mo_index_idx_col ColDef, causing the storage layer to not recognize it as a sort key. This meant index table data was unsorted during flush/merge, zone maps had maximal overlap, and prefix_in block filter could not prune any blocks. With this fix, newly created secondary index tables have their index column marked as primary, enabling sorted data layout and effective block-level zone map filtering. Refs matrixorigin#24241 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jiangxinmeng1
changed the title
…or sort key" This reverts commit d21128a.
heni02
pushed a commit
that referenced
this pull request
Apr 29, 2026
## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: fixes #24241 ## What this PR does / why we need it: - Cherry-pick of #24246 to 3.0-dev branch - Add `UnpackNthElement` to deserialize only the Nth element from a serialized tuple, avoiding full deserialization - Add fast path in `serial_extract` for constant index parameter (the common case in DISTINCT queries on secondary index) - **7.9x speedup** on DISTINCT queries over composite secondary index (14.9s → 1.9s aggregate phase on 5M rows) ## Adaptation for 3.0 - Removed `yearCode` case (not present in 3.0) - Adapted nil test case (3.0 Packer has no `EncodeNull`) ## Test plan - [x] Unit test `TestUnpackNthElement` passes - [x] Local verification: 10M row table with 6-column composite index, DISTINCT aggregate 1209ms (vs ~15s without fix) - [x] Builds successfully on 3.0-dev 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: XuPeng-SH <xupeng3112@163.com>
- Cover all type branches in UnpackNthElement (int8/16/32/64, uint8/16/32/64, bool, float32/64, date, datetime, timestamp, time, year, decimal64/128, varchar, bit, objectid, uuid, nil, error paths) - Add TestSerialExtractConstIndex to exercise the new constant-index fast path in serialExtractExceptStrings and serialExtractForString - Test null p1, null tuple elements, and normal extraction for both string and non-string result types Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
|
/auto-test-pr |
ouyuanning
approved these changes
Apr 30, 2026
aunjgr
approved these changes
Apr 30, 2026
XuPeng-SH
approved these changes
Apr 30, 2026
Contributor
Merge Queue Status
This pull request spent 2 minutes 6 seconds in the queue, with no time running CI. Waiting for:
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks HintYou may have to fix your CI before adding the pull request to the queue again. |
Collaborator
|
/auto-test-pr |
Collaborator
PR #24246 测试覆盖分析变更摘要本 PR 优化了 6 类测试覆盖情况
图例: ✅ 已覆盖 建议
🤖 自动生成的测试 PR✅ 大数据 PR 已提交：https://github.com/Ariznawlll/mo-nightly-regression/pull/5 由 AI Test Analyzer 自动生成 · workflow run |
Contributor
Merge Queue Status
This pull request spent 20 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Which issue(s) this PR fixes:
fixes #24241
What this PR does / why we need it:
UnpackNthElementto decode only up to the target element in a serialized tuple, instead of deserializing the entire blobserial_extractwhen the index parameter is constant (the common case in GROUP BY on composite secondary index)serial_extractCPU overhead ~6x for DISTINCT queries on leading columns of composite secondary indexesBackground
When running
SELECT DISTINCT col FROM table WHERE col IN (...)on the leading key of a composite secondary index, the query usesserial_extract(__mo_index_idx_col, 0, VARCHAR)as the GROUP BY key. The old implementation deserialized the entire composite tuple for every row just to extract the first element, causing ~35s of CPU overhead on 45M rows.With this optimization, only the first element is decoded, reducing the aggregate phase from ~171ms to ~28ms on 100K rows (6x improvement).
Test plan
TestUnpackNthElement— verifies correctness for all types, nil values, single elements, out-of-rangeTestSerialExtract— existing test still passesTestSimpleTupleAllTypes— existing tuple tests pass🤖 Generated with Claude Code