c-thiel commented on code in PR #2188:
URL: https://github.com/apache/iceberg-rust/pull/2188#discussion_r3357161256
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -421,6 +425,67 @@ impl Schema {
pub fn field_id_to_fields(&self) -> &HashMap<i32, NestedFieldRef> {
&self.id_to_field
}
+
+ /// Returns the minimum [`FormatVersion`] required to represent all types
in this schema.
+ ///
+ /// Defaults to `FormatVersion::V1` if all types are universally supported.
+ pub fn min_format_version(&self) -> FormatVersion {
Review Comment:
Rolling back:
I tried the `SchemaVisitor` approach first but moved to a small
leaf_min_format_version(&Type) helper + iteration over the flattened
id_to_field. Three reasons it's cleaner here:
1. Infallibility vs. the trait. `SchemaVisitor` returns Result on every
method, but this logic can't fail — so it needed .expect("never fails") at the
call sites.
1. Wrong shape for the check: `check_format_compatibility` wants a shallow,
per-field test (each flattened field judged by its own type, so blame lands on
the leaf, not its container). The visitor is a recursive tree-folder; reusing
it meant calling `visit_type` on single leaves behind an `!is_nested()` guard —
indirect, and it silently couples that guard to which visitor arms count as
"leaf."
1. Cost for no benefit. The visitor allocated a `Vec<FormatVersion>` per
struct level in `min_format_version` (the flat fold allocates nothing) and was
~40 lines of 7-method boilerplate for a 5-line rule.
The flat form is also closer to Java, which keeps this as a static
`MIN_FORMAT_VERSIONS` map in Schema iterated over lazyIdToField() — not a
visitor. So: one match as the single source of truth, both `min_format_version`
and `check_format_compatibility` route through it, and the per-field iteration
mirrors `checkCompatibility` directly.
Fixed in
https://github.com/apache/iceberg-rust/pull/2188/commits/ef25e2ccbd12f70be5af5b8c99b046409c88b13e
--
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]
