moomindani commented on PR #2802: URL: https://github.com/apache/iceberg-rust/pull/2802#issuecomment-5726456693
Thanks for going through this — two of these were real, and I've pushed fixes for both. **`equivalent_ignoring_names`**: you were right that it needed revisiting. It compared only `source_id`, which for a multi-argument field holds just the first id, so two fields sharing a field id and a first source id but reading different columns compared equal and the cross-spec compatibility check accepted them. It now compares the effective source ids. Worth noting for anyone reading later: a spec-compliant multi-argument field normalizes to `Transform::Unknown` and `compute_unified_partition_type` rejects unknown transforms before this point, so the gap is reachable through direct construction or a field that carries both `source-id` and `source-ids`, not through the ordinary read path. **The builders**: this turned out to be worse than an ergonomics gap. `UnboundPartitionField` had no `source_ids` at all, so `From<PartitionField>` dropped the extra ids — and `UnboundPartitionSpec` is the wire type for `TableCreation` and `TableUpdate::AddSpec`, so a v3 multi-argument spec silently degraded to single-argument on the way back out. `source_ids` is now on `UnboundPartitionField` and threaded through both conversions, and its serde shares `normalize_transform_sources` with the bound field so the spec's `source-ids`-only form deserializes too. Adding a multi-argument setter to `add_partition_field` itself is a separate ergonomics change I'd rather do once the read side is settled. **Table metadata tests**: added, via a `TableMetadataV3MultiArgTransforms.json` fixture that pins both a multi-argument partition field and a multi-argument sort field, read and written back. **`repartition.rs`**: those two lines are `source_ids: None` in struct literals inside `mod tests` — required for the crate to compile, not new functionality in the DataFusion integration. **Spec-version-explicit serde**: I left this as is for now. `source-ids` is v3-only, `skip_serializing_if` keeps v1/v2 output byte-identical, and the version-dependent normalization lives in one shared function. Java has no multi-argument implementation to mirror, so there's no reference shape to match yet. Happy to restructure it along `schema/_serde.rs` lines if a committer prefers that. -- 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]
