feat: stabilize field IDs across schema evolution - #8658
Conversation
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
|
Do you think this will cause transactions to conflict with each other that did not before? I don't think it does today since we treat any schema change as conflicting with any other schema change. Do we know why we reused field ids in the first place? |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Agreed that stable field IDs are fundamentally a writer-side contract. The reader bit was only intended to fence legacy writers, so I’ll keep activation explicit and align the commit-authoritative allocator with Composite Transactions’ temporary-ID model. |
wjones127
left a comment
There was a problem hiding this comment.
I haven't gotten to the implementation yet, just read the format changes. I'm glad it's a writer flag only.
The prose is pretty difficult to read. I asked Claude to help suggest how to rewrite it, taking inspiration from the ASD-STE100 writing standard.
I also have some questions
| schema is merged into an activated dataset, IDs for newly introduced logical fields are cleared and | ||
| assigned by the dataset allocator; callers cannot select or reserve IDs through Arrow metadata. | ||
|
|
||
| The presence of `Manifest.max_allocated_field_id` is the activation marker. If it is absent, the |
There was a problem hiding this comment.
question(blocking): what's the difference between setting this field and setting the writer flag? What happens if you set this field but not the writer flag? What happens if you set the writer flag, but not this field?
There was a problem hiding this comment.
The field stores allocator state, while the flag gates writers; both must be set together, and either mismatch is invalid.
| - **Reorder Columns**: Change field order in schema; IDs remain the same | ||
| - **Type Evolution**: Data type can be changed. This might require rewriting the column in the data, depending on how the type was changed. | ||
| - **Metadata or Nullability Change**: Preserve the field ID | ||
| - **Type Replacement**: Allocate a new field ID and retire the old identity |
There was a problem hiding this comment.
question(non-blocking): is this new behavior or just a description of existing behavior? I wonder if this is really desireable.
There was a problem hiding this comment.
This is existing cast behavior: changing the logical type creates a new field identity, so it receives a new ID.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
1 fixed / 1 new. The field/flag validity contract is now explicit, but this revision removes the retire-before-migration precondition based on the resolved compatibility discussion. #8580 added generic writer-feature checks only in v12.0.0-beta.4; older writers can still open this writer-only-gated dataset, and their generic schema commits did not reject or preserve the unknown stable-ID state. Such a writer can republish an activated table without its high-water contract and re-enable ID reuse. Explicit migration changes who opts in, not the need to quiesce those writers first.
Restore the requirement to retire pre-gate writers before migrate_to_stable_field_ids, both in the public API documentation and the operator-facing format/versioning documentation.
wjones127
left a comment
There was a problem hiding this comment.
The spec looks a lot better. Reviewed the implementation this time.
| The writer may temporarily add `kind`, `blob_id`, `blob_size`, and `position`. A Lance data file | ||
| stores this descriptor shape: | ||
|
|
||
| ```python | ||
| pa.schema([ | ||
| pa.field( | ||
| "image", | ||
| pa.struct([ | ||
| pa.field("kind", pa.uint8(), nullable=False), | ||
| pa.field("position", pa.uint64(), nullable=False), | ||
| pa.field("size", pa.uint64(), nullable=False), | ||
| pa.field("blob_id", pa.uint32(), nullable=False), | ||
| pa.field("blob_uri", pa.string(), nullable=False), | ||
| ]), | ||
| ), | ||
| ]) | ||
| ``` |
There was a problem hiding this comment.
question(non-blocking): when these fields are stored, how is that represented in both DataFile.fields and DataFile.column_indices? Below it says DataFile.fields = [0]. Are they really missing from the DataFile metadata? If so, how does a reader know where in the Lance file to find those columns?
There was a problem hiding this comment.
praise: this looks a lot better! Nice work.
|
|
||
| #[tokio::test] | ||
| async fn test_raw_arrow_overwrite_preserves_reordered_stable_field_identities() { | ||
| let source_uri = TempStrDir::default(); |
There was a problem hiding this comment.
nitpick: For tests that aren't validating disk IO, I prefer to use the memory:// URI.
| let schema = Arc::new(ArrowSchema::new(vec![ | ||
| ArrowField::new("a", DataType::Int64, false), | ||
| ArrowField::new("b", DataType::Int64, false), | ||
| ])); | ||
| let batch = RecordBatch::try_new( | ||
| schema.clone(), | ||
| vec![ | ||
| Arc::new(Int64Array::from(vec![1, 2])), | ||
| Arc::new(Int64Array::from(vec![3, 4])), | ||
| ], | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
nitpick: prefer to use record_batch!() macro for simple data, 1
| let schema = Arc::new(ArrowSchema::new(vec![ | |
| ArrowField::new("a", DataType::Int64, false), | |
| ArrowField::new("b", DataType::Int64, false), | |
| ])); | |
| let batch = RecordBatch::try_new( | |
| schema.clone(), | |
| vec![ | |
| Arc::new(Int64Array::from(vec![1, 2])), | |
| Arc::new(Int64Array::from(vec![3, 4])), | |
| ], | |
| ) | |
| .unwrap(); | |
| let batch = record_batch!( | |
| ("a", Int64, [1, 2]), | |
| ("b", Int64, [3, 4]) | |
| ).unwrap(); |
Footnotes
| let err = validate_operation(Some(&manifest), &reused).unwrap_err(); | ||
| assert!( |
There was a problem hiding this comment.
suggestion: could we also assert which error variant this is emitting? I think it would be InvalidInput, right?
| let Some(restored_max_field_id) = manifest.max_allocated_field_id else { | ||
| return Err(Error::invalid_input(format!( | ||
| "Cannot restore version {version}: stable field IDs were activated after that version" | ||
| ))); | ||
| }; |
There was a problem hiding this comment.
question: remind me, why can't we restore? What bad happens if we let users restore to a previous state?
| /// Canonicalize schema identities supplied by a transaction before validation. | ||
| /// | ||
| /// Arrow field-ID metadata is descriptive input, not allocation authority. New | ||
| /// datasets allocate from zero, while stable datasets preserve compatible | ||
| /// existing identities and allocate every new identity above the persisted | ||
| /// high-water mark. File mappings written against the incoming schema are | ||
| /// updated in the same step; files retained by a merge are never rewritten. |
There was a problem hiding this comment.
suggestion: it's unclear what "canonicalize" means. Or what "schema identities" means. Could we just call this "Assign field ids given an operation". And then we can list the rules below that for how field ids are assigned?
"allocate", "identity", "canonicalize" are all new words we haven't really used. "Assign a field id" is something familiar.
| let mut current_id = i64::from(schema_max_id.max(max_existing_id)) + 1; | ||
| let unassigned_count = self.fields_pre_order().filter(|field| field.id < 0).count() as i64; | ||
| if unassigned_count > 0 && current_id + unassigned_count - 1 > i64::from(i32::MAX) { | ||
| return Err(Error::invalid_input( | ||
| "No further field ID can be allocated because IDs are exhausted", | ||
| )); | ||
| } |
There was a problem hiding this comment.
suggestion: could we keep in the i32 domain by using saturating arithmetic?
| let mut current_id = i64::from(schema_max_id.max(max_existing_id)) + 1; | |
| let unassigned_count = self.fields_pre_order().filter(|field| field.id < 0).count() as i64; | |
| if unassigned_count > 0 && current_id + unassigned_count - 1 > i64::from(i32::MAX) { | |
| return Err(Error::invalid_input( | |
| "No further field ID can be allocated because IDs are exhausted", | |
| )); | |
| } | |
| let mut current_id = schema_max_id.max(max_existing_id).saturating_add(1); | |
| let unassigned_count = self.fields_pre_order().filter(|field| field.id < 0).count() as i32; | |
| if unassigned_count > 0 && (current_id - 1).saturating_add(unassigned_count) > i32::MAX { | |
| return Err(Error::invalid_input( | |
| "No further field ID can be allocated because IDs are exhausted", | |
| )); | |
| } |
| return Err(Error::invalid_input( | ||
| "No further field ID can be allocated because IDs are exhausted", | ||
| )); |
There was a problem hiding this comment.
suggestion: it would be nice to not have to traverse the whole schema up front to discover this. Couldn't we discover this while we are allocating ids? I suppose the downside is we would error right when we are in the middle of allocating ids, and leave the schema in an in-between state.
There was a problem hiding this comment.
We actually do check it when allocating. Do we really need this then?
| /// Transient schema-metadata marker used by bindings for raw Arrow input. | ||
| pub const TRANSACTION_SCHEMA_SOURCE_RAW_ARROW: &str = "lance:transaction_schema_source_raw_arrow"; |
There was a problem hiding this comment.
question: what's the purpose of this? When is this necessary?
Field IDs are physical bindings in Lance, but legacy allocation derives the next ID from fields that the current snapshot still references. After a field and its files disappear, a later field can reuse the same integer. A bare field ID is therefore not a durable identity across schema evolution.
This PR adds an explicit, writer-side stable field-ID contract:
migrate_to_stable_field_ids.max_allocated_field_idandFLAG_STABLE_FIELD_IDS. Both values must be present for stable mode or absent for legacy mode; either mismatch is invalid. Stable field IDs do not set a reader flag.The migration API is Rust-only in this PR. Python and Java enforce the contract when they operate on an activated dataset, but they do not expose activation yet.
The format documentation also defines how Blob logical fields participate in dataset field identity while writer-prepared and stored descriptor children remain file-local representation details.
Validation includes Rust stable/legacy migration, schema evolution, retry/rebase, restore, clone, detached commit, Blob identity, and Arrow canonicalization coverage, plus Java Merge, Project, and Overwrite boundary coverage.