Kurtiscwright commented on code in PR #2417:
URL: https://github.com/apache/iceberg-rust/pull/2417#discussion_r3327257256
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -421,6 +421,52 @@ impl Schema {
pub fn field_id_to_fields(&self) -> &HashMap<i32, NestedFieldRef> {
&self.id_to_field
}
+
+ /// Returns the minimum table format version required by any type in this
schema.
+ ///
+ /// Returns [`None`] when the schema contains no type with a specific
minimum
+ /// table format version requirement.
+ pub fn min_format_version(&self) -> Option<FormatVersion> {
+ self.id_to_field
+ .values()
+ .filter_map(|field| field.field_type.as_primitive_type())
+ .filter_map(PrimitiveType::min_format_version)
+ .max()
+ }
+
+ /// Validates that all schema types are supported by the table format
version.
+ pub fn validate_compatible_with_format_version(
Review Comment:
It would be nice to have a 4+ layer deep unit test and an integration test
with 100+ columns and multiple deeply nested structs to see how this
validations affects Table creation times.
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -421,6 +421,52 @@ impl Schema {
pub fn field_id_to_fields(&self) -> &HashMap<i32, NestedFieldRef> {
&self.id_to_field
}
+
+ /// Returns the minimum table format version required by any type in this
schema.
+ ///
+ /// Returns [`None`] when the schema contains no type with a specific
minimum
+ /// table format version requirement.
+ pub fn min_format_version(&self) -> Option<FormatVersion> {
+ self.id_to_field
+ .values()
+ .filter_map(|field| field.field_type.as_primitive_type())
+ .filter_map(PrimitiveType::min_format_version)
+ .max()
+ }
+
+ /// Validates that all schema types are supported by the table format
version.
+ pub fn validate_compatible_with_format_version(
+ &self,
+ format_version: FormatVersion,
+ ) -> Result<()> {
+ let problems = self
+ .id_to_field
+ .values()
+ .filter_map(|field| {
+ let field_type = field.field_type.as_ref();
+ let primitive = field_type.as_primitive_type()?;
+ let min_format_version = primitive.min_format_version()?;
+ (format_version < min_format_version).then(|| {
Review Comment:
Do you need to do a comparison here? I thought the point of implementing the
match in `primitive.min_format_version()` was so it would do the comparison for
you.
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -421,6 +421,52 @@ impl Schema {
pub fn field_id_to_fields(&self) -> &HashMap<i32, NestedFieldRef> {
&self.id_to_field
}
+
+ /// Returns the minimum table format version required by any type in this
schema.
+ ///
+ /// Returns [`None`] when the schema contains no type with a specific
minimum
+ /// table format version requirement.
+ pub fn min_format_version(&self) -> Option<FormatVersion> {
+ self.id_to_field
+ .values()
+ .filter_map(|field| field.field_type.as_primitive_type())
+ .filter_map(PrimitiveType::min_format_version)
+ .max()
+ }
+
+ /// Validates that all schema types are supported by the table format
version.
+ pub fn validate_compatible_with_format_version(
+ &self,
+ format_version: FormatVersion,
+ ) -> Result<()> {
+ let problems = self
+ .id_to_field
+ .values()
+ .filter_map(|field| {
+ let field_type = field.field_type.as_ref();
+ let primitive = field_type.as_primitive_type()?;
+ let min_format_version = primitive.min_format_version()?;
+ (format_version < min_format_version).then(|| {
+ let column_name =
self.name_by_field_id(field.id).unwrap_or(field.name.as_str());
Review Comment:
Is it possible to cause this `unwrap_or` to throw an error? I believe that
would cause a panic and a crash at runtime, but I can't figure out if its
possible to pass a `field.name` that would cause the `as_str()` to panic.
--
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]
