Repository navigation
fix(arrow): strip metadata from list elements and map entries - #3303
NoahKusaba wants to merge 6 commits into
Conversation
strip_metadata_from_schema failed with "Field stack underflow in list" on any schema with a list or map column. MetadataStripVisitor pushed a field onto its stack only in before_field, which the visitor does not call for list elements, map keys or map values, so rebuilding those popped from an empty stack. Push in before_list_element, before_map_key and before_map_value too. The stack now holds each field's name and nullability rather than a Field with a placeholder type, and one pop_field helper rebuilds fields for struct, list, map and primitive. Document the two normalizations the function makes besides removing metadata, which callers comparing stripped schemas rely on: a map's entries field is renamed to DEFAULT_MAP_FIELD_NAME, and a dictionary-encoded field becomes its value type. Closes apache#3297
|
cc @mbutrovich, @comphead |
comphead
left a comment
There was a problem hiding this comment.
Two small docs points.
ArrowSchemaVisitor::before_fieldandafter_fieldare documented as called around a "struct/list/map field". The traversal calls them for every schema and struct field, primitives included, and never for list elements or map keys and values, which have their own hooks. A visitor that pushes inbefore_fieldonly, likeMetadataStripVisitordid, underflows on lists and maps. Consider rewording to "Called before each field of the schema or of a struct, whatever its type. List elements and map keys and values usebefore_list_element,before_map_keyandbefore_map_valueinstead." and mirroring it onafter_field.- The comment on
test_strip_metadata_renames_map_entriessays a map from arrow-rs or DataFusion compares equal to a table's once stripped. That holds for DataFusion'smap()(entries,key,value), but not for an arrow-rsMapBuilderdefault (entries,keys,values), as the test's ownkeysandvaluesfixture shows. Rewording it to say only the entries field is renamed would match what the test asserts.
| /// Two other differences are normalized away too, so that schemas that differ only in | ||
| /// them compare equal once stripped: | ||
| /// - A map's entries field is renamed to [`DEFAULT_MAP_FIELD_NAME`] and made non-nullable, | ||
| /// as [`schema_to_arrow_schema`] builds it. Its key and value fields keep their names. |
There was a problem hiding this comment.
This says map key and value names are kept, but it is silent on list element names. Are they kept on purpose? arrow-rs names list elements item (Field::new_list_field) and schema_to_arrow_schema names them element, so a stripped List(item) still differs from a stripped table List(element).
If keeping them is the intent, one more sentence here would complete the contract. If callers need them normalized, LIST_FIELD_NAME is the name to rebuild with. That may not matter for datafusion-iceberg if DataFusion already casts the input to the table's nested names, which is unverified.
There was a problem hiding this comment.
Keeping them is intentional: renaming list elements would change what callers see, and the map rename only exists to match schema_to_arrow_schema. I added a paragraph in 1980676 that says all other names are kept, including list elements, and calls out the item vs element case.
comphead
left a comment
There was a problem hiding this comment.
Thanks @NoahKusaba it looks good to me, some nits
These depend on iceberg-rust's strip_metadata_from_schema handling list and map columns (apache/iceberg-rust#3303) and fail until that lands.
…ma naming before_field and after_field run for every schema and struct field, not for list elements or map keys and values. strip_metadata_from_schema keeps list element names, and renames only a map's entries field.
Which issue does this PR close?
What changes are included in this PR?
strip_metadata_from_schemafailed withField stack underflow in liston any schema with a list or map column.MetadataStripVisitorpushes each field's name and nullability onto a stack inbefore_fieldand pops it when rebuilding the field. For list elements and map keys and values, the traversal callsbefore_list_element,before_map_keyandbefore_map_valueinstead ofbefore_field, so rebuilding them popped from an empty stack. The visitor now pushes in those three hooks too.While here:
Fieldwith aDataType::Nullplaceholder, and onepop_fieldhelper rebuilds fields forstruct,list,mapandprimitive.DEFAULT_MAP_FIELD_NAME, asschema_to_arrow_schemanames it, and a dictionary-encoded field becomes its value type. Callers comparing stripped schemas rely on both. For example, datafusion-iceberg compares an INSERT's input, whose map entries field DataFusion namesentries, against the table's schema.Are these changes tested?
Three unit tests in
arrow::schema, each comparing the whole stripped schema with the expected one:test_strip_metadata_from_nested_schema: lists, large and fixed-size lists, sorted and unsorted maps, and a list of structs holding a map of lists, with metadata on the schema and every field. Fails without the fix with the error from the issue.test_strip_metadata_renames_map_entriestest_strip_metadata_unwraps_dictionariesAI Disclosure