Kurtiscwright commented on code in PR #2417:
URL: https://github.com/apache/iceberg-rust/pull/2417#discussion_r3319681125
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1565,6 +1573,25 @@ impl Display for FormatVersion {
}
}
+/// Returns the minimum table format version required by any type in a schema.
+///
+/// Returns [`None`] when the schema contains no type with a specific minimum
+/// table format version requirement.
+pub fn min_format_version_for_schema(schema: &Schema) -> Option<FormatVersion>
{
Review Comment:
Should this move to schema.rs? That way the validation lives where the data
is defined.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -196,6 +199,41 @@ impl TableMetadataBuilder {
)
}
+ fn validate_schema_compatible_with_format_version(
Review Comment:
Doesn't this tie validation to only create or updating Tables? Should this
validation happen on all usages of the schema, including during serde?
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -60,6 +60,14 @@ pub const MIN_FORMAT_VERSION_ROW_LINEAGE: FormatVersion =
FormatVersion::V3;
/// Reference to [`TableMetadata`].
pub type TableMetadataRef = Arc<TableMetadata>;
+static PRIMITIVE_TYPE_MIN_FORMAT_VERSION: LazyLock<HashMap<PrimitiveType,
FormatVersion>> =
Review Comment:
This feels very Java-y. I would consider moving to the PrimitiveType trait
and using `match` so we get a compile time check when new data types are added.
Example of this would be:
```rust
impl PrimitiveType {
/// Returns the minimum table format version required by this
primitive type,
/// or `None` if the type is supported in all format versions.
pub const fn min_format_version(&self) -> Option<FormatVersion> {
match self {
Self::TimestampNs | Self::TimestamptzNs =>
Some(FormatVersion::V3),
Self::Boolean
| Self::Int
| Self::Long
| Self::Float
| Self::Double
| Self::Decimal { .. }
| Self::Date
| Self::Time
| Self::Timestamp
| Self::Timestamptz
| Self::String
| Self::Uuid
| Self::Fixed(_)
| Self::Binary => None,
}
}
}
```
The implementation above would mean `Self::Variant` triggers a compile time
error until it gets added to this check. If we ever have a data type specific
to version > 3 than we would just add an arm above the `V3` line.
--
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]
