fix(readutil): preserve decimal scale in zonemap filters - #26815
Conversation
|
Independent owner-review found a P1 before merge; this draft is being reworked. The new scale-aware helpers currently return false for an uninitialized DECIMAL zonemap. On a nullable sorted/cluster key, an all-NULL block can legitimately have an uninitialized zonemap. In the Required closure:
PR remains draft and should not be accepted at |
|
Scope update: baseline black-box testing proves the uninitialized sorted-zonemap problem is generic, not DECIMAL-only. It reproduces with nullable BIGINT and VARCHAR cluster keys ( |
|
Follow-up Validation:
No partition scenario was used. CI is not being monitored. |
|
Five-PR live combination acceptance passed on 55:
Important interaction found during staging: the four-way candidate without #26806 made the base/shadow DECIMAL path correct via #26815 while the secondary-index range path remained wrong ( |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Follow-up deep review completed on fb3879c58df after rebasing the full PR onto latest main at db2ef4dd03.
The prior P1 is closed. The fail-open contract now covers scalar comparisons, equality, BETWEEN/in_range, IN, and all prefix variants at object, block, quick-break, and sorted-seek layers. Unknown/uninitialized or type-incompatible zonemaps cannot exclude data. Decimal IN membership also requires compatible scale before zonemap/Bloom rejection, while typed min/max bounds retain safe cross-scale quick-break/seek. Sorted seek validates the object zonemap and every block zonemap before binary search.
The follow-up review also closed two adjacent prefix-vector unhappy paths: malformed or non-VARCHAR payloads do not compile, and nullable prefix vectors conservatively skip zonemap pruning/seek rather than entering the non-null sorted-vector fast path.
Regression coverage now includes DECIMAL64/128, BIGINT, VARCHAR, IN, prefix_eq/prefix_between/prefix_in_range hints 0-3/prefix_in, unknown leading blocks, incompatible metadata, scale-mismatched vectors, nullable prefix vectors, malformed payloads, and an unsampled unknown block that would otherwise make binary search unsafe.
Validation on the rebased tree:
- controlled CGo full tests:
./pkg/container/types ./pkg/vm/engine/readutilPASS - focused fail-open/type/null regressions PASS
go build -mod=readonlyandgo vet -mod=readonlyfor both packages PASSgit diff --checkPASS- seek benchmark remains 0 allocs/op; 256 blocks about 0.98 us on Apple M4
No remaining code-review findings in the complete PR diff. GitHub does not permit approving my own PR, so this is recorded as a COMMENT review. CI was not awaited.
What type of PR is this?
Which issue(s) this PR fixes:
issue #26808
issue #26817
What this PR does / why we need it:
DECIMAL fast filters retained folded literal bytes but lost the literal's scale. Object and block zonemap comparisons then interpreted a lower-scale bound as though it used the persisted column scale, which could prune matching blocks and return wrong rows.
This change preserves the bound type/scale in a typed zonemap and uses scale-aware comparisons for object filters, block filters, sorted quick-break/seek, equality,
BETWEEN, andin_rangehints 0–3. Incompatible comparison metadata fails open. DECIMAL Bloom lookups skip unsafe raw-byte rejection when type or scale differs. It also makesCompareDecimal64WithScalepromote to DECIMAL128 when scaling overflows or crosses the signed range.It also makes unknown/uninitialized sorted zonemaps fail open instead of treating them as proof to stop scanning. This closes the NULL-first nullable
CLUSTER BYfalse-pruning path reproduced with DECIMAL, BIGINT, and VARCHAR.Validation includes:
<,<=,>,>=,=,BETWEEN, andin_range0–3No partitioned table is involved.