[4.2-dev] Backport #29513: composite range pruning and DECIMAL256 safety - #29518
Conversation
## What type of PR is this? - [x] BUG - [x] Improvement ## Which issue(s) this PR fixes: Fixes matrixorigin#29507 ## What this PR does / why we need it: A range on the first component of a composite primary or cluster key did not produce a hidden sort-key predicate for object pruning. The scan could load metadata for objects whose sort-key zone map already ruled them out. The planner adds a supplemental hidden-key block filter for compatible bounded ranges, `BETWEEN`, and one-sided comparisons, including reversed operands. The original SQL row predicate remains authoritative. The change uses the existing composite-key serializer, `BlockFilterList`, and readutil's sort-key zone-map fast path. First-component paired bounds stay separate, avoiding a premerge cast that could change bound semantics. Block filters now retain metadata-only column references without adding those columns to the scan reader. Row-needed columns keep their compact positions; omitted metadata columns receive distinct positions for the combined runtime/block zonemap path. This also removes unnecessary row reads for existing composite-part block filters. The relation's full schema still supplies physical sequence numbers and zonemaps; no new reader, storage format, or persistent state is introduced. Partition pruning also consumes block filters. It now matches named predicates to the partition column instead of assuming scan-local and stored partition positions are equal. Ambiguous dotted alias/column names conservatively skip partition pruning. The readutil debug diagnostic likewise resolves by column name rather than indexing the full schema with a scan-local position. The leading-range optimization remains limited to supported integer, temporal, and same-scale decimal bounds. Float and byte-string leading ranges retain the original scan path because signed zero or byte-prefix ordering could otherwise cause false-negative pruning. Some first-component `BETWEEN` plans on those types therefore lose their former hidden-key rewrite; exact row filtering remains in place. ## Validation - Full CGo package tests passed for `pkg/sql/plan`, `pkg/sql/compile`, `pkg/partitionprune`, and `pkg/vm/engine/readutil`. - Public SQL-to-plan tests verify that hidden compound and old constituent metadata filters remain effective while block-only columns are absent from reader attributes. A combined runtime/block column map test checks distinct physical sequence numbers. - `FilterObjects` tests verify object rejection before metadata loading. Partition tests cover same-column compaction, different-column position collision, and dotted-name ambiguity. - Incremental `go vet` and `golangci-lint` passed; `git diff --check` passed. Against base `a36da7f72d`, the final diff has production net +210 lines, tests net +389, and no document changes. The planner mapping and partition identity checks account for the production increase; the tests exercise public planning and pruning behavior rather than only helper internals. The historical 329-object local reproduction establishes the missed early-pruning opportunity but did not reproduce the reported production latency. Deterministic tests establish object rejection and removal of hidden-key reader attributes; service-level latency and byte savings have not been measured. CI is pending. (cherry picked from commit 76862df)
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? |
P2 — DECIMAL256 的前缀范围优化资格与 4.2 serializer 能力不一致审查提交: 已通过实际 SQL 和 planner/runtime-folding 回归确认:该回移使原本可以执行的查询报错,空表即可复现。建议修复后再合并。 复现在已选择的测试数据库中执行: CREATE TABLE d(a DECIMAL(40,0), b INT) CLUSTER BY(a,b);
SET @x = 1;
EXPLAIN SELECT b FROM d WHERE a > CAST(@x AS DECIMAL(40,0));
SELECT b FROM d WHERE a > CAST(@x AS DECIMAL(40,0));
-- literal CAST 也可复现
SELECT b FROM d WHERE a > CAST(1 AS DECIMAL(40,0));
根因
这是优化准入与实际执行能力不一致导致的查询可用性回归;当前证据不是数据丢失或误裁剪。 建议
main 与 4.2 的区别核对当前 main |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review — ee54c414323535dbed33d941ac0d397f723ebdce
结论:本轮未发现剩余阻塞问题,建议合并。受影响的历史 DECIMAL256 索引仍必须按升级文档维护重建后再开放查询。
审查基线为最新核对的 4.2-dev / 1a44f3db3056e32975fde86ca0ab69a094996a3f。单 agent 完成,未修改生产代码,未等待 CI。按用户要求排除 Iceberg。当前登录账号也是 PR 作者,使用 COMMENT 登记评审结论,不能提交正式 APPROVE。
设计与工程质量
- 完整追查 leading-range planner → serial/folding → metadata/object/block filter;原 SQL 行谓词保留,类型、scale、列身份和 volatile bounds 的准入约束一致。第一列范围的收益有对象在 metadata load 前被排除的确定性计数测试。
- DECIMAL256 补齐已有 tuple 编码、提取、长度契约和 legacy 索引压缩职责,沿用已有 33-byte 格式。检查了 preinsertunique/preinsertsecondaryindex、索引回查、hashbuild/runtimefilter 等直接消费者;NULL 过滤与主键行对齐不能分开成立。
- 列身份复用现有
ColRef.TblName,统一物理名称解析,清理重复 first-dot 解析及 scan position 等于物理列位置的旧假设。metadata-only 列保持独立,不增加行读取列;partition pruning 按真实分区列匹配。 serial_full的 NULL→值复用与动态提取负索引已有明确错误/复用回归;packer、fold executor 和索引 buffer 的 reset/free 继续由现有执行器和 operator 负责。未发现新增重复状态机、持久状态或执行框架。- 分别审查了增量:实现 +428/-111(净 +317);测试/结果 +1352/-9(净 +1343),另有两个 Parquet 夹具;文档/运维 SQL +74。新测试区分编码、NULL 压缩、身份、分区、缺失统计和真实导入路径,数据量小;既有大数据 blockfilter setup 未扩张。测试行数未作为免审依据。
QA 与证据
本轮新增 reviewer-only 真实 SQL 挑战,使用单 CN、六行数据,最终 PASS 14.10 秒,Go 进程退出码 0,包含 vet:
- 检查
mo_ctl(...flush...)JSON 返回Flush/OK。 - EXPLAIN 明确断言
Index Table Scan on t.bi、INDEX backfill join,以及原生 DECIMAL256 主键t.a的 runtime filter;IGNORE INDEX 对照计划没有索引扫描。 - 大于 DECIMAL128 范围的正负主键;prepared NULL→值→非法值→有效值复用。
- 修改主键后的事务内索引回查、ROLLBACK 恢复、提交并 flush 后的回查。
- 重复主键 UPDATE 拒绝后数据与索引保持一致;DELETE 后 flush、NULL companion 行保留。
- 索引/基础表完整结果对照同时有显式期望 ID。
最初仅写 FORCE INDEX 的小数据查询实际走基础表,因此该轮 PASS 未计作索引回查证据。补充 EXPLAIN guard 后保留了失败日志,最终采用 ORDER BY b 触达并断言实际回查路径。诊断源文件已归档并从工作树移除,工作树保持 clean。
复用了 当前 head 已完成的 CI:Ubuntu UT race、shared build、SCA、pessimistic 与 proxy multi-CN BVT 成功;日志确认选中 optimizer/composite_range_decimal.test、optimizer/blockfilter.test、join/leftjoin.sql。原始 old-writer/new-reader/rebuild、math/big 与同实例两轮 BVT 证据来自 PR body 的作者记录,本轮没有重新执行这些作者实验;记录中的历史失败未计作 PASS。
Comments 与部署边界
最新核对 0 个 review threads;唯一技术 P2 评论 的 unsupported-DECIMAL256 故障已解决。作者采用完整补齐既有支持的方案,literal/runtime bound 的 folding 与公开 SQL 回归已有覆盖,后续索引写入和缺失统计消费者也纳入修复。
两个边界需要保留:
- 旧 legacy writer 省略的索引组件不能靠 decoder 恢复。按 升级文档 做全账户 inventory、保存 DDL、维护窗口从基础表重建;失败时保持入口关闭,回滚恢复完整升级前备份。合并代码不自动修复历史索引。
- 原生 DECIMAL256 缺少可用 min/max zone map 时保守扫描;这解决误裁剪,扫描成本仍存在。复合 VARCHAR 键可继续裁剪,不能把原生列的保守回退宣传为性能提升。
What type of PR is this?
Which issue(s) this PR fixes:
Related to #29507: first-column range on a composite sort key misses object-level pruning.
Backport of #29513 (merged commit
76862df5c89e8868bed5963c428f65b9eafb34b3) to4.2-dev, validated against base1a44f3db3056e32975fde86ca0ab69a094996a3f. This updates the existing backport; it does not close an additional issue.What this PR does / why we need it:
Composite leading-range pruning preserves the SQL row predicate while pruning compound keys. The backport also keeps metadata scan slots separate from physical column identity, resolves partition predicates against the actual partition column, and honors
CannotFold()so volatile bounds cannot independently evaluate in row and prefix filters.The repair completes DECIMAL256 support instead of disabling its optimization:
serial,serial_full,SerialHelper, extraction, and encoded-length contracts. Keep decimal scales compatible while allowing ordinary integer literals to reach leading-range pruning.serial_fullpack functions lazily after a NULL argument becomes non-NULL, including reuse/reset. Reject negative dynamic extraction indices in both reverse consumers.ColRef.TblNamefield through remapping, copying and protobuf round trips, so aliases and physical column names containing dots resolve correctly without changing EXPLAIN formatting. Metadata and row-level primary-key consumers share the same physical-name resolver; obsolete first-dot parsing and scan/relation position assumptions are removed.Upgrade requirement
Existing indexes written by a legacy DECIMAL256 writer require rebuilding from the base table in a maintenance window before user reads/writes resume. This is not an online rolling-upgrade protocol. Ordinary indexed LOAD often rejected DECIMAL256, but FK LOAD can reach a legacy writer that omitted components or rows; the reader cannot reconstruct those missing values from old keys.
The delivered upgrade procedure documents backup, stopping all old CNs and writers, per-account inventory, preserving original index definitions, rebuilding, verification and rollback. The inventory SQL conservatively lists regular indexes on DECIMAL256 tables, including indexes that use a DECIMAL256 PK suffix. The inventory also includes RTREE/SPATIAL; it does not auto-execute destructive DDL.
The upgrade was challenged with the exact original PR production sources: FK Parquet LOAD succeeded under the old binary; the repaired reader on the same data directory returned no row through the old index while the base scan returned the qualifying row. Existing DROP/CREATE rebuilt the index from the base table, restored matching results, and passed UPDATE/DELETE/NULL checks. Typed single-column UNIQUE is also checked as an unaffected control.
Validation
All commands used Go 1.26.4,
GOWORK=off, readonly module resolution, the worktree's own native build and an explicit build temporary directory.Own native build:
make -j8 cgo; final service:make build— PASS.Full ordinary owning/direct-consumer tests via
.agents/skills/mo-dev/scripts/mo-cgo-test -p=2 -v -count=1 -timeout=600s:pkg/sql/plan/function,pkg/sql/plan,pkg/partitionprune,pkg/vm/engine/readutil,pkg/sql/util,pkg/sql/colexec/hashbuild,pkg/sql/colexec/runtimefilter— PASS.After adding the index compaction repair: full
pkg/sql/utilwith-vet=all, plus fullpkg/sql/colexec/preinsertuniqueandpkg/sql/colexec/preinsertsecondaryindexordinary tests — PASS. The three new index tests were observed failing before their production repair and passing afterward.Full vet for the seven original packages — PASS. Incremental configured golangci-lint over those packages plus both preinsert consumers,
--new-from-rev 1a44f3db3056e32975fde86ca0ab69a094996a3f— PASS, 0 issues. The initial lint attempt timed out; it is not counted as a pass. The bounded retry used lower concurrency and completed. The three index packages were also rechecked after their production sources stabilized: PASS, 0 issues.Serializer tests cover signed physical extrema, cross-word values, ordering, round trips, NULL contracts, reset/reuse and invalid extraction indices. Planner/readutil tests cover actual folding, integer scale metadata, dotted aliases, protobuf identity and truncated compound zonemaps without false negatives.
Persisted optimized/reference comparison: 96 comparisons across bigint/DECIMAL64/128/256,
<,<=,>,>=, three bounds and ordinary/dotted aliases — PASS. This ran before the final index-only compaction additions; those additions have separate UT/BVT coverage.Production binary for commit 7cf (SHA256
51cbf5d71b64edc8919f65698aa170bc23bf07fe41622840771c1b226e15292f), fresh one-CN local service, fixed golden two normal BVT rounds on the same instance, with no issue-skipping flag:optimizer/composite_range_decimal.test78/78,optimizer/blockfilter.test55/55,join/leftjoin.sql68/68; 201/201 each round, no ignored or abnormal statements. All non-built-in database/table catalog counts were zero between rounds and afterward. The new 78-statement golden and two Parquet fixtures were independently checked against SQL contracts before accepting the comparison results.Reusing an older test directory failed WAL replay (
IOEntry[3004,2]); the exact original PR binary failed identically on that directory. Both failures were retained as failures. The final two BVT rounds used a fresh directory and completed.Independent QA and final incremental review for the earlier repair: gpt-6.1-sol, xhigh, approved tree
4b171afd8fbc437121f5547695b1711faf544a74/ delivered commite0c6bb5871c658c1fc394665814141df9e45e994. All identified production blockers were repaired and re-reviewed. No new concurrency owner, worker, wait or unbounded cache was introduced; no new full race-suite pass is claimed. The prior backport's full planner race timeout on both candidate and clean base remains historical non-pass evidence.Subsequent deep review, performed by the main agent without subagents as requested, delivered commit
7cf0409fa04888725223b2dc1335d377fd3977ac/ reviewed tree0b87c487a75e88d95c177a7ef411b43440b420bf: unify both row-PK consumers with the existing qualifier resolver; reduce production code by 14 lines; remove dead eligibility test logic; require fold errors and cleanup; cover invalid extraction in both decimal and string decoders; make the maintenance sequence executable by saving inventory/DDL before stopping old CNs.After the test cleanup, full ordinary planner/function/readutil packages passed again (4.231s/13.390s/1.947s), configured three-package incremental lint passed with 0 issues. After the final row-PK source edit, full readutil tests with
-vet=allpassed again (1.942s), and configured readutil incremental lint passed with 0 issues. Six dotted-alias subcases failed before the PK repair; all 18 identity/input-shape subcases passed afterward, including non-PK controls and different scan/relation positions.Commit 7cf production binary, independent public SQL oracle: 59 assertions passed. This repeats the exact DECIMAL(40,0) comment on empty/non-empty tables, checks persisted fractional DECIMAL256 comparisons with real dotted column/alias names, both invalid VARCHAR extraction consumers, atomic rollback after duplicate-key batches in both orders, nullable unique rows, and real simple-PK equality/IN/range/non-PK controls. The two BVT rounds above were also repeated after this last production edit.
Comments were re-read: the single technical issue comment is addressed by complete DECIMAL256 support and its exact SQL reproduction. The other comment says automated review is paused; it is not an approval.
Adversarial black-box / white-box QA follow-up
Delivered commit
ee54c414323535dbed33d941ac0d397f723ebdce, reviewed tree56911cd3a2ae06fbea088e37f18caaeeac95caf4; main-agent QA, without subagents as requested. Historical independent approval above applies to its named tree, not to this new tree.Found and repaired a wrong-result path: a six-row raw DECIMAL256 primary-key table with an integer UNIQUE index returns a row through FORCE INDEX before flush, but drops it after flush while the base scan returns it. Both ordinary INSERT and FK Parquet LOAD reproduced it, including small values; the exact original PR production binary also reproduced INSERT + flush. This is a pre-existing gap that full DECIMAL256 support must close.
Raw DECIMAL256 intentionally has no initialized min/max zonemap because its values do not fit the existing layout. Runtime IN during index back-lookup incorrectly treated those absent statistics as no match. A 21-line guard in the existing readutil compiler conservatively retains raw DECIMAL256 blocks/objects and leaves exact row filtering in charge. AND can still prune using other supported columns; OR cannot discard the unknown branch. Serialized VARCHAR compound-key pruning remains enabled. No new format, state machine, storage path, or concurrency owner is introduced.
Final service binary SHA256
8e58b686cda62a198bd2587e71d250bca3664c010f0ff7ec11af5109651f426f:The large-test draft initially passed BLOB input from UNHEX to serial_extract, failing type checking before decoding. The final test casts to the accepted VARCHAR input and verifies actual truncated/invalid markers, constant/row extraction and encoded NULL; only that final result is counted. Historical WAL replay failures and baseline failures remain failures. No new throughput benchmark or full race pass is claimed.
Iceberg CI is explicitly excluded by the requester. On previous commit
7cf0409fa04888725223b2dc1335d377fd3977ac, run36674804116passed preflight, shared build, SCA, UT coverage, Coverage, Mongo, PROXY and PESSIMISTIC. Ubuntu UT failed only atpkg/tests/features.TestTableFeaturesshared three-CN cluster initialization (waitAnyShardReadyLocked/context deadline exceeded), before table-feature assertions; this is retained as a failure rather than declared unrelated without a base comparison. A targeted localTestTableFeaturesrun with an isolated TMPDIR passed on the new production tree; this does not prove a clean-base comparison or turn the failed remote job green. The new SHA must receive its own CI results. No new full-repository CI success is claimed. The corresponding main-branch volatile-bound fix remains a separate follow-up.