CTTY commented on code in PR #2188:
URL: https://github.com/apache/iceberg-rust/pull/2188#discussion_r3222845045
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -419,6 +420,56 @@ 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.
+ ///
+ /// Iterates over every field and returns the highest minimum version
among them,
+ /// defaulting to `FormatVersion::V1` if all types are universally
supported.
+ pub fn min_format_version(&self) -> FormatVersion {
+ self.id_to_field
+ .values()
+ .map(|f| f.field_type.min_format_version())
+ .max()
+ .unwrap_or(FormatVersion::V1)
+ }
+
+ /// Check that all types in this schema are supported by the given format
version.
+ ///
+ /// Mirrors Java's `Schema.checkCompatibility()`. Returns an error listing
every
+ /// incompatible field if any are found.
+ ///
+ /// Types with a minimum format version:
+ /// - `TimestampNs` / `TimestamptzNs` → v3+
+ /// - `Variant` → v3+
+ pub fn check_format_compatibility(&self, format_version: FormatVersion) ->
Result<()> {
+ let mut problems: Vec<String> = Vec::new();
+
+ for field in self.id_to_field.values() {
+ let min_version = field.field_type.min_format_version();
+
+ if format_version < min_version {
+ let name = self
+ .name_by_field_id(field.id)
+ .unwrap_or(field.name.as_str());
+ problems.push(format!(
+ "Invalid type for {name}: {} is not supported until
{min_version} but format version is {format_version}.",
+ field.field_type,
+ ));
+ }
+ }
+
+ if problems.is_empty() {
+ Ok(())
+ } else {
+ Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Invalid schema for v{format_version}:\n- {}",
Review Comment:
```suggestion
"Invalid schema for {format_version}:\n- {}",
```
nit: `v` seems redundant
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -42,6 +42,11 @@ pub const MAP_KEY_FIELD_NAME: &str = "key";
/// Field name for map type's value.
pub const MAP_VALUE_FIELD_NAME: &str = "value";
+/// Minimum format version required for nanosecond-precision timestamp types
(v3).
+pub const MIN_FORMAT_VERSION_TIMESTAMP_NS: FormatVersion = FormatVersion::V3;
+/// Minimum format version required for the variant type (v3).
+pub const MIN_FORMAT_VERSION_VARIANT: FormatVersion = FormatVersion::V3;
Review Comment:
nit: These are not really needed, since it's hardcoded to V3 anyway
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -749,6 +750,15 @@ impl TableMetadata {
Ok(())
}
+
+ /// Validates that every type used across all schemas is supported by the
+ /// table's format version. Delegates to
[`Schema::check_format_compatibility`].
+ fn validate_schema_format_compatibility(&self) -> Result<()> {
+ for schema in self.schemas.values() {
Review Comment:
Could you help me understand why do we need to validate all schemas rather
than just the current 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]
