fix(index): preserve zone map correctness for all data types - #8190
fix(index): preserve zone map correctness for all data types#8190wirybeaver wants to merge 3 commits into
Conversation
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
4878e14 to
4b3e593
Compare
4b3e593 to
0271a1c
Compare
0271a1c to
9e3d816
Compare
westonpace
left a comment
There was a problem hiding this comment.
This is a great follow-up, a nice extensive set of tests (I appreciate that it even has a migration test) and I think a worthy use of a new version.
It is a spec change, so we will need a vote. I'll get that started and try to come back later and look at this with more detail.
| #[derive(Clone, Copy)] | ||
| #[repr(u32)] | ||
| enum ZoneMapIndexVersion { | ||
| Ordered = 0, | ||
| NullOnly = 1, | ||
| } |
There was a problem hiding this comment.
Are you saying that version 1 is only used for the null-only case? In other words, a writer will choose version 0 or version 1 based on the data type?
I think we want version numbers to be more of an increasing, inclusive concept. In other words...
Version 0 does not know how to create null-only zone maps.
Version 1 can create everything version 0 can and can also do null-only zone maps.
Is this a correct understanding?
There was a problem hiding this comment.
Yes, that is the intended compatibility model. ZoneMapIndexPlugin::version() reports the maximum version this implementation supports (1), while each created index records the minimum reader version required by its physical layout. Ordered indices therefore continue to write version 0, and null-only indices write version 1. A version-1 implementation can read and create both layouts; a version-0 reader retains ordered indices and ignores null-only indices so it falls back to scanning.
| fn serialize_data_type(data_type: &DataType) -> Result<bytes::Bytes> { | ||
| let schema = Arc::new(arrow_schema::Schema::new(vec![Field::new( | ||
| "value", | ||
| data_type.clone(), | ||
| true, | ||
| )])); | ||
| let mut buffer = Cursor::new(Vec::new()); | ||
| let mut writer = FileWriter::try_new(&mut buffer, &schema)?; | ||
| writer.finish()?; | ||
| Ok(bytes::Bytes::from(buffer.into_inner())) | ||
| } |
There was a problem hiding this comment.
Why do we need to store the data type in the index?
There was a problem hiding this comment.
Null-only zone maps store min and max physically as Arrow Null, so their file schema no longer carries the indexed logical type. The scalar-index loader receives the index store and details, but not the dataset field, and the logical type is needed after loading to validate updates and rebuild the correct processor/seeds. The data_type global buffer preserves it as a one-field Arrow IPC schema. Ordered version-0 maps still infer the type from min/max and do not write this buffer.
| Field::new("null_count", DataType::UInt32, false), | ||
| Field::new("nan_count", DataType::UInt32, false), | ||
| Field::new("zone_length", DataType::UInt64, false), | ||
| Field::new("null_offsets", DataType::Binary, false), |
There was a problem hiding this comment.
null_offsets contains the exact top-level-null row positions relative to the start of each seed zone, encoded as little-endian u64 values in the binary field. null_count alone is insufficient to reconstruct the complete RowAddrTreeMap when an append/update is built from seeds. During seed loading, each zone-relative offset is combined with the zone start and fragment ID to recover the absolute row address. Legacy seeds without this field deliberately fall back to the scanned update path.
9e3d816 to
cdc8283
Compare
8700a32 to
edbee84
Compare
Keep dense-null seed payloads compact while validating decoded offsets and documenting zone span and null-query guarantees.
edbee84 to
20b88ff
Compare
There was a problem hiding this comment.
🟡 Gate recommendation: maintainer decision required.
The rebase preserves ordered version-0 compatibility while isolating null-only maps in version 1; no independent code blocker remains.
Maintainers still need to decide whether to adopt Zone Map Version 1: version 1 enables the explicit null-only layout and exact null pruning for non-orderable types while older readers safely fall back to scans. Retaining version-0-only behavior avoids the new stable layout but forgoes that capability. The required format vote still has no recorded votes or result.
Summary
This is now a focused follow-up to #8017. It preserves that PR's merged nested-type enablement, Python validation removal, FSL scalar-index discovery, index naming fixes, and scalar/vector coexistence coverage while adding the remaining correctness and compatibility work:
Nullextrema columns while leaving existing ordered maps on version 0Design Decisions
ZoneMapMode::{Ordered, NullOnly}. Convert to the protobufsupports_min_maxboolean only at the persistence boundary; an absent field means ordered mode for legacy details.lance_arrow_stats::supports_ordered_extrema, rather than only checking Arrow nesting or physical representation.minandmaxcolumns physically as ArrowNullarrays. Persist the indexed logical type separately in adata_typeglobal buffer encoded as an Arrow IPC schema.RowAddrTreeMapas the complete top-level null bitmap. Child nulls and empty collections do not make the containing row null.IN, and value-range behavior.Relationship to #8017
The remaining differences and their rationale are summarized in a follow-up comment on #8017. This branch is rebased on its merge commit and intentionally does not duplicate or replace its Python/planner changes.
Related to #7987.
Test Plan
cargo fmt --allcargo check --workspace --tests --benchescargo clippy --all --tests --benches -- -D warningscargo test -p lance-arrow-stats(83 passed; 3 doctests passed)cargo test -p lance-index --no-fail-fast(1020 passed; 2 ignored; 7 doctests passed)cargo test -p lance test_dataset_null_only_zonemap_for_list_column --no-fail-fastcargo test -p lance test_released_zonemap_fixture_preserves_ordered_behavior --no-fail-fast