You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@iceberg.apache.org by "fvaleye (via GitHub)" <gi...@apache.org> on 2025/08/20 08:15:36 UTC

[PR] fix(schema): check if new schema field names conflict with existing partition field names [iceberg-rust]

fvaleye opened a new pull request, #1619:
URL: https://github.com/apache/iceberg-rust/pull/1619

   ## Which issue does this PR close?
   - Closes #1609
   
   ## What changes are included in this PR?
   Previously, the Iceberg Rust implementation only validated partition field names against schema field names when creating partition specs, but did not validate the reverse direction during schema evolution. This meant users could add schema fields that conflicted with existing partition field names, which could cause confusion and errors.
   
   ## Are these changes tested?
   Yes, with unit tests


-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] fix(schema): check if new schema field names conflict with existing partition field names [iceberg-rust]

Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.

liurenjie1024 commented on code in PR #1619:
URL: https://github.com/apache/iceberg-rust/pull/1619#discussion_r2287710743


##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -679,7 +682,54 @@ impl TableMetadataBuilder {
 
     /// Add a schema and set it as the current schema.
     pub fn add_current_schema(self, schema: Schema) -> Result<Self> {
-        self.add_schema(schema).set_current_schema(Self::LAST_ADDED)
+        self.add_schema(schema)?
+            .set_current_schema(Self::LAST_ADDED)
+    }
+
+    /// Check if schema field names conflict with existing partition field names.
+    ///
+    /// This validation prevents introducing new field names that match existing partition field names
+    /// that don't correspond to any field in the current schema. Field names that already exist in the
+    /// current schema are allowed since partition fields are typically derived from schema fields.
+    ///
+    /// # Errors
+    /// - Schema field name conflicts with existing partition field name that doesn't correspond to any current schema field.
+    fn check_schema_partition_name_conflicts(&self, schema: &Schema) -> Result<()> {
+        let existing_partition_field_names: HashSet<&str> = self

Review Comment:
   This may be too wasteful to build a hash set for it. `Schema` already contains mapping from field name to field ids, we could go through parttion field names and lookup it in schema.



##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########


Review Comment:
   We should also do this check when adding spec?



##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -679,7 +682,54 @@ impl TableMetadataBuilder {
 
     /// Add a schema and set it as the current schema.
     pub fn add_current_schema(self, schema: Schema) -> Result<Self> {
-        self.add_schema(schema).set_current_schema(Self::LAST_ADDED)
+        self.add_schema(schema)?
+            .set_current_schema(Self::LAST_ADDED)
+    }
+
+    /// Check if schema field names conflict with existing partition field names.
+    ///
+    /// This validation prevents introducing new field names that match existing partition field names
+    /// that don't correspond to any field in the current schema. Field names that already exist in the
+    /// current schema are allowed since partition fields are typically derived from schema fields.
+    ///
+    /// # Errors
+    /// - Schema field name conflicts with existing partition field name that doesn't correspond to any current schema field.
+    fn check_schema_partition_name_conflicts(&self, schema: &Schema) -> Result<()> {
+        let existing_partition_field_names: HashSet<&str> = self
+            .metadata
+            .partition_specs
+            .values()
+            .flat_map(|spec| spec.fields().iter().map(|field| field.name.as_str()))
+            .collect();
+
+        let current_schema_field_names: HashSet<&str> = self

Review Comment:
   This is incorrect, nested schema have more fields than first struct layer. You could use `schema.field_id_to_name_map().values` to go through all names.



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org