NoahKusaba commented on code in PR #3303:
URL: https://github.com/apache/iceberg-rust/pull/3303#discussion_r4238397571
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -1320,6 +1310,17 @@ impl ArrowSchemaVisitor for MetadataStripVisitor {
/// including nested struct, list, and map fields. This is useful for schema
comparison
/// where metadata differences should be ignored.
///
+/// 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.
+/// - A dictionary-encoded field becomes a field of the dictionary's value
type.
+///
+/// All other field names are kept, including a map's key and value fields and
a list's
Review Comment:
Thanks, agreed that the half-and-half was the confusing part. In 4ccf45609 I
went with normalizing: list elements are renamed to `LIST_FIELD_NAME`, map keys
and values to `MAP_KEY_FIELD_NAME`/`MAP_VALUE_FIELD_NAME`, and the map key is
made non-nullable, so every container field now matches
`schema_to_arrow_schema`.
`test_strip_metadata_normalizes_container_field_names` builds the same schema
with arrow-rs names (`item`, `entries`/`keys`/`values`, nullable key) and from
an Iceberg schema through `schema_to_arrow_schema`, and checks that both strip
to the same result. Nullability is still kept everywhere else, so a nullable
`item` won't match a required table element, which I think is right.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -1320,6 +1310,17 @@ impl ArrowSchemaVisitor for MetadataStripVisitor {
/// including nested struct, list, and map fields. This is useful for schema
comparison
/// where metadata differences should be ignored.
///
+/// 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.
+/// - A dictionary-encoded field becomes a field of the dictionary's value
type.
Review Comment:
Added a doc line saying extension type metadata is dropped with the rest, so
a variant becomes a plain struct of its `metadata` and `value` fields.
`test_strip_metadata_turns_variants_into_structs` checks this for a variant at
the top level, in a list and in a map. I kept dropping it, since both sides of
the comparison go through the same strip.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -2600,4 +2601,135 @@ mod tests {
pretty_assertions::assert_eq!(schema, expected);
assert_eq!(schema.highest_field_id(), 17);
}
+
+ #[test]
+ fn test_strip_metadata_from_nested_schema() {
+ // The same schema, built with and without metadata on the schema and
every field.
+ let schema = |with_metadata: bool| {
+ let metadata = |value: &str| {
+ if with_metadata {
+ HashMap::from([("k".to_string(), value.to_string())])
+ } else {
+ HashMap::new()
+ }
+ };
+ let field = |name: &str, ty: DataType, nullable: bool| {
+ Field::new(name, ty, nullable).with_metadata(metadata(name))
+ };
+ let element = || Arc::new(field("element", DataType::Int32,
false));
+ let map = |sorted| {
+ let entries = Fields::from(vec![
+ field("key", DataType::Utf8, false),
+ field("value", DataType::List(element()), true),
+ ]);
+ let entries = field(DEFAULT_MAP_FIELD_NAME,
DataType::Struct(entries), false);
+ DataType::Map(Arc::new(entries), sorted)
+ };
+ let nested = Fields::from(vec![field("m", map(false), true)]);
+ ArrowSchema::new(vec![
Review Comment:
Added `map<utf8, struct<..>>` and `list<list<..>>` to the nested fixture,
and folded the `a.b.a.b` repeated-names case into it as well. The `item` vs
`element` negative case no longer applies, since those names are now
normalized. Instead, `test_strip_metadata_keeps_other_differences` checks that
a different struct field name or list element nullability still compares
unequal.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -1190,63 +1194,74 @@ pub(crate) fn
primitive_type_to_arrow_type_with_ree(primitive_type: &PrimitiveTy
/// including nested struct, list, and map fields. This is useful for schema
comparison
/// where metadata differences should be ignored.
struct MetadataStripVisitor {
- /// Stack to track field information during traversal
- field_stack: Vec<Field>,
+ /// Fields whose types are still being visited, innermost last
+ pending_fields: Vec<PendingField>,
+}
+
+/// What `MetadataStripVisitor` keeps of a field until its type is visited
+struct PendingField {
+ name: String,
+ nullable: bool,
}
impl MetadataStripVisitor {
fn new() -> Self {
Self {
- field_stack: Vec::new(),
+ pending_fields: Vec::new(),
}
}
+
+ /// Records a field whose type is about to be visited.
+ fn start_field(&mut self, field: &FieldRef) -> Result<()> {
+ self.pending_fields.push(PendingField {
+ name: field.name().clone(),
+ nullable: field.is_nullable(),
+ });
+ Ok(())
+ }
+
+ /// Builds the innermost pending field with `data_type` and no metadata.
+ /// `kind` names the type in the error for an unbalanced stack.
+ fn finish_field(&mut self, data_type: DataType, kind: &str) ->
Result<Field> {
Review Comment:
Done: `schema()` now returns an `Unexpected` error if fields are left on the
stack after the traversal, alongside the underflow error in `finish_field`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
