laskoviymishka commented on code in PR #3303:
URL: https://github.com/apache/iceberg-rust/pull/3303#discussion_r4232814866
##########
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:
The normalization is asymmetric in a way that'll surprise callers: we rename
the map entries field to `key_value` but leave the list element name alone, so
a `List<item>` from DataFusion or arrow-rs still won't match a table's
`List<element>`. Same story for map key/value — arrow-rs `MapBuilder` emits
`keys`/`values` — and the key's required-ness isn't normalized either. The
motivating datafusion-iceberg INSERT still gets a spurious schema mismatch on
any list column; it just fails later than the crash used to.
I'd pick one rule: either normalize the list element and map key/value names
(and the key nullability) to the `schema_to_arrow_schema` set, or keep this
metadata-only and align names in the caller. The half-and-half is the confusing
part. If the list-element case is deliberately out of scope for #3297, let's
say so in the PR so it's tracked rather than reading like an oversight.
##########
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:
The fixture's solid, but the original bug was an off-by-one in paired
push/pop hooks, so the cases most likely to catch a regression are containers
nested inside another container's push window — `map<string, struct<..>>` (the
map-value push interleaving with the struct's children) and `list<list<..>>`.
Both are cheap with the builder closures already here. A negative case would
help too: the doc claims schemas differing only in `item` vs `element` stay
unequal, but nothing asserts that a genuine mismatch stays unequal.
##########
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:
While we're listing what gets normalized — stripping also drops
`ARROW:extension:name`, so a variant column (`arrow.parquet.variant`) collapses
to a plain `struct<metadata, value>` once stripped. Harmless for the datafusion
comparison since both sides go through the same strip, but it's now reachable
for variants nested in lists/maps and nothing here documents or tests it. I'd
add a bullet noting extension-type metadata is dropped and a variant-in-list
case to pin it — or preserve `ARROW:extension:*` if we want variants to stay
distinct from plain structs.
##########
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:
`finish_field` catches an underflow, but the opposite — a field left on
`pending_fields` after traversal — is silent today. A cheap
`pending_fields.is_empty()` check at the end of `schema()` (returning an error,
not a panic) would catch leaks as well as underflows and make the
one-pop-per-node invariant enforce itself. Optional, but it's the kind of thing
that turns the next regression into a clear error instead of a quietly wrong
schema.
--
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]
