fix(global-index): stop reading index keys with a comparator that can panic - #909
Conversation
… panic
`make_key_comparator` picks its arm from the column's current type and is then
handed keys written when the index was built. Nothing records the type it was
built with: neither `IndexManifestEntry` nor `IndexFileMeta` carries a schema id,
and `UpdateColumnType` guards partition, primary-key, bucket-key and
primary-key-index columns but not global-index columns. So `ALTER TABLE t ALTER
COLUMN id TYPE BIGINT` on a btree-indexed INT column is accepted, and the next
`WHERE id = 7` panics with "range end index 8 out of range for slice of length 4"
in `key_serde.rs`, inside the DataFusion scan. `TIMESTAMP(3)` to `TIMESTAMP(6)`
reaches the same place through the varint decode, and the multivalue path reaches
it through the ARRAY element type.
`KeyComparator` now returns `Result<Ordering>` and every arm checks the width it
is about to read, so no stored key can crash a query. A failure means the bytes
are not keys of this type, so the index cannot answer:
- `all_matching_entries` is a pure optimisation, so the entry is not an
all-match;
- `may_match` / `may_match_between` prune, so they answer "may match" -- never
"cannot match", which would drop rows;
- a failed index query declines the entry, and the predicate falls through to
the read pipeline the same way an unsupported operator does.
`KeyComparisonFailure` keeps this apart from a real I/O failure where the two
share `io::Result`. On the build side both keys come from the current type, so a
failure there aborts the build instead of degrading.
Out of scope: a cast whose new encoding accepts the stored key's width still
reads those bytes as the new type and can answer from the wrong values -- INT to
FLOAT, BIGINT to DOUBLE, a DECIMAL scale change, and any numeric to
DECIMAL(p > 18), where the BigInteger form accepts 1 to 16 bytes and so accepts
every fixed-width key this module writes. Those give a wrong ordering rather than
a panic, and telling a foreign key from a real one needs the type the index was
built with, which is not recorded anywhere.
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Reviewed head 1daea1b. Requirement fit: supported; implementation: no blocking findings for the stated panic fix. I traced the width mismatch through file-level pruning (conservatively "may match"), the reader comparison error, and evaluate_leaf declining the index so the ordinary read pipeline applies the predicate. Locally, cargo test -p paimon --lib btree::key_serde::tests --features fulltext,vortex passed (5 tests), and cargo test -p paimon-datafusion --test global_index_schema_evolution passed (the indexed INT -> BIGINT SQL regression). The documented same-width type/scale changes can still silently misinterpret an older index; that is a separate correctness gap worth tracking with build-time type/schema metadata or index invalidation, rather than treating this width-specific panic fix as general schema-evolution safety.
2 participants
A sorted global index is read with a comparator built from the column's current type, but its keys were written with the type it had at build time — and no index file records a schema id.
UpdateColumnTypeguards partition, primary-key, bucket-key and primary-key-index columns, not global-index ones.So
ALTER TABLE t ALTER COLUMN id TYPE BIGINTon a btree-indexedINTcolumn is accepted, and the nextWHERE id = 7panics withrange end index 8 out of range for slice of length 4inside the scan.TIMESTAMP(3)→TIMESTAMP(6)and the multivalue ARRAY element type reach the same place.KeyComparatornow returnsResult<Ordering>and each arm checks the width it reads, so no stored key can crash a query. A failure means the index cannot answer: the all-match shortcut reports nothing,may_matchanswers "may match" rather than "cannot match", and a failed query declines the entry so the predicate falls through to the read pipeline. On the build side a failure aborts the build.Out of scope: a cast whose new encoding accepts the stored key's width still reads those bytes as the new type —
INT→FLOAT, aDECIMALscale change, any numeric→DECIMAL(p > 18)(BigInteger accepts 1 to 16 bytes). Those give a wrong ordering, not a panic; separating a foreign key from a real one needs the build-time type, which is not recorded.