fix(dataset): validate nested schema names and stored system columns - #9499
lance-gatefixer[bot] wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
Keep the recursive sibling-name validation, but reserve system-column names only at the schema root, where virtual fields are injected. This closes the stored-field collision while preserving schema changes and restore for previously valid nested fields.
| /// ``` | ||
| pub fn validate_writable(&self) -> Result<()> { | ||
| self.validate()?; | ||
| for field in self.fields_pre_order() { |
There was a problem hiding this comment.
This rejects outer._rowid even though virtual system columns are added only at the schema root. Lance v12.0.0 allowed that nested field in a stored schema, so an existing dataset now fails schema-changing commits that leave the field intact; after renaming the field, restoring its earlier version fails through write_manifest_file with InvalidInput instead of rolling back. Keep the recursive sibling-name check, but inspect only self.fields for reserved system names.
Reproducer run on this head
Add this to schema.rs's test module and run cargo test -p lance-core --lib gate_nested_rowid_remains_writable:
#[test]
fn gate_nested_rowid_remains_writable() {
let child = ArrowField::new("_rowid", DataType::UInt64, false);
let arrow = ArrowSchema::new(vec![ArrowField::new(
"outer",
DataType::Struct(vec![child].into()),
false,
)]);
let schema = Schema::try_from(&arrow).unwrap();
schema.validate_writable().unwrap();
}The test fails with InvalidInput: The column '_rowid' at path 'outer._rowid' is a reserved name. The nested field should pass this stored-schema check; a separate integration probe confirmed the restore failure.
There was a problem hiding this comment.
Addressed in 6641a46: writable schemas now reserve system names only at the root. The rename test covers a nested _rowid through a schema change that preserves it and a restore.
There was a problem hiding this comment.
Fixed in 6641a462f: writable-schema validation now checks reserved names only at the root. The updated rename test keeps a nested _rowid through another schema change and restores the earlier version.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The shared manifest write boundary now rejects stored top-level system names across schema-changing commits, and recursive sibling-name validation prevents ambiguous nested paths. The follow-up preserves a nested _rowid through schema changes and restore, resolving the compatibility finding.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The new empty-name check closes the reported write/read failure: a stored root field with no name cannot be addressed by field-path parsing. Validation runs at the shared manifest boundary, with an earlier check in add_columns before staging. The recursive sibling checks and root-only system-name rule remain intact, including nested _rowid schema changes and restore.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The merge keeps both per-parent sibling-name validation and main's row-lineage ID handling. Stored root system and empty names remain rejected at the manifest write boundary, while nested system names remain usable through schema changes and restore.
Summary
Schema::validate()checked duplicate names only at the root, so two children of the same struct could share a name. Selected writers checked reserved system-column names, allowing schema evolution and prewritten-fragment commits to store a top-level field that collides with a virtual column. The Issue also showed that writers accepted an empty top-level name even though field-path parsing cannot address it afterward.Validate sibling names with a per-parent set throughout the schema. At the manifest write boundary,
Schema::validate_writable()rejects all five reserved system names and empty names at the root.add_columnsvalidates its output names before staging data because field-path resolution can run before the manifest check. Nested system names remain writable: virtual columns are injected only at the root, and previously written datasets can still change schema and restore.A schema-changing commit rejects an existing top-level reserved or empty name. Datasets already written with an empty root name still need to be rewritten from their accessible columns.
Related draft #7854 covers newly introduced top-level reserved names; this repair also covers duplicate nested sibling names and the distributed commit path.
Validation
cargo test -p lance-core --lib validate_writablecargo test -p lance-core --doc validate_writablecargo test -p lance --lib rejects_unwritable_top_level_column_namescargo test -p lance --lib test_append_columns_exprscargo test -p lance --lib test_rename_columnscargo fmt --allcargo clippy --all --tests --benches -- -D warningsFixes #9455