diff --git a/rust/lance/src/io/commit.rs b/rust/lance/src/io/commit.rs index 27329daf482..4e4d48e368a 100644 --- a/rust/lance/src/io/commit.rs +++ b/rust/lance/src/io/commit.rs @@ -830,10 +830,16 @@ fn fix_schema(manifest: &mut Manifest) -> Result<()> { seen_fields.clear(); } - // Apply mapping to the schema + // Apply mapping to the schema. Children carry an independent `parent_id` + // that is serialized verbatim, so re-parent them onto the new id as well. + // A field's children are named by this field's id only, so each entry can + // be applied independently of the order the mapping is iterated in. for (old_field_id, new_field_id) in &old_field_id_mapping { let field = manifest.schema.mut_field_by_id(*old_field_id).unwrap(); field.id = *new_field_id; + for child in field.children.iter_mut() { + child.parent_id = *new_field_id; + } } // Drop data files that are no longer in use. @@ -2524,6 +2530,95 @@ mod tests { assert_eq!(manifest.fragments.as_ref(), &expected_fragments); } + #[test] + fn test_fix_schema_nested() { + // A duplicated struct field is renumbered, so its children must be + // re-parented onto the new id. Otherwise the manifest serializes a + // parent_id that no longer exists and can never be read back. + let mut field0 = Field::try_from(ArrowField::new("a", DataType::Int64, false)).unwrap(); + field0.set_id(-1, &mut 0); + + let inner = ArrowField::new( + "inner", + DataType::Struct(vec![ArrowField::new("ordinal", DataType::Int32, false)].into()), + false, + ); + let mut outer = Field::try_from(ArrowField::new( + "s", + DataType::Struct(vec![ArrowField::new("text", DataType::Utf8, false), inner].into()), + false, + )) + .unwrap(); + // ids: s = 1, text = 2, inner = 3, ordinal = 4 + outer.set_id(-1, &mut 1); + + let schema = Schema { + fields: vec![field0, outer], + metadata: Default::default(), + }; + // Both the struct (1) and the nested struct (3) have duplicate coverage + // within this fragment, so both get renumbered. + let fragments = vec![Fragment { + id: 0, + files: vec![ + DataFile::new_legacy_from_fields("path1", vec![0, 1, 2, 3, 4], None), + DataFile::new_legacy_from_fields("path2", vec![1, 3], None), + ], + overlays: vec![], + deletion_file: None, + row_id_meta: None, + physical_rows: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + }]; + + let mut manifest = Manifest::new( + schema, + Arc::new(fragments), + DataStorageFormat::default(), + HashMap::new(), + ); + + fix_schema(&mut manifest).unwrap(); + + // max_field_id was 4, so 1 -> 5 and 3 -> 6. + let outer = &manifest.schema.fields[1]; + assert_eq!(outer.id, 5); + let text = &outer.children[0]; + let inner = &outer.children[1]; + assert_eq!( + text.parent_id, 5, + "child of a renumbered struct kept a stale parent_id" + ); + assert_eq!( + inner.parent_id, 5, + "child of a renumbered struct kept a stale parent_id" + ); + assert_eq!(inner.id, 6); + assert_eq!( + inner.children[0].parent_id, 6, + "grandchild of a renumbered struct kept a stale parent_id" + ); + + // Every non-root field must name a parent that still exists in the schema. + let live_ids = manifest + .schema + .fields_pre_order() + .map(|f| f.id) + .collect::>(); + for field in manifest.schema.fields_pre_order() { + if field.parent_id >= 0 { + assert!( + live_ids.contains(&field.parent_id), + "field '{}' (id={}) references parent id {}, which no longer exists", + field.name, + field.id, + field.parent_id + ); + } + } + } + /// A CommitHandler that always fails with OtherError, used to simulate /// a manifest write failure so we can verify orphaned transaction files /// are cleaned up.