Re: [PR] feat: check schema capability when adding parquet files [iceberg-rust]

["jdockerty (via GitHub)" <gi...@apache.org>]

jdockerty commented on code in PR #2158:
URL: https://github.com/apache/iceberg-rust/pull/2158#discussion_r2890720584


##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -317,6 +321,19 @@ impl ParquetWriter {
         // TODO: support adding to partitioned table
         let mut data_files: Vec<DataFile> = Vec::new();
 
+        let table_schema = table_metadata.current_schema();
+        let name_mapping = table_metadata
+            .properties()
+            .get(DEFAULT_SCHEMA_NAME_MAPPING)
+            .map(|s| serde_json::from_str::<NameMapping>(s))
+            .transpose()

Review Comment:
   Is this `transpose` usage required?
   
   If I'm understanding this rightly, it is being used to flip the `map` output on `Err` (after `from_str`) for the `map_err` to bubble up afterwards. 
   
   Although it seems like some indirection when a `match` would be more explicit/easy to follow too.
   
   
   This may just be a preference thing though, feel free to disregard 👍 
   



##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -431,6 +431,133 @@ impl Display for Schema {
     }
 }
 
+/// Check whether `file_schema` is compatible with `table_schema` for appending data files.
+///
+/// Walks the table schema and checks that every field is present and type-compatible
+/// in the file schema (looked up by field ID). Follows the same semantics as
+/// iceberg-python's `_check_schema_compatible`.

Review Comment:
   I'm not sure how likely it is that this method name would change, but it is perhaps worth linking to the specific commit that you've referenced this from?
   
   That way the details are clear from looking at the reference implementation.



##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -250,6 +250,34 @@ pub enum PrimitiveType {
 }
 
 impl PrimitiveType {
+    /// Returns `true` if `self` can be promoted to `target` per the Iceberg spec's
+    /// valid type promotion rules.
+    ///
+    /// See <https://iceberg.apache.org/spec/#schema-evolution>

Review Comment:
   Great reference 💯 



-- 
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