mbutrovich commented on issue #3524: URL: https://github.com/apache/iceberg-rust/issues/3524#issuecomment-6665423598
> Secondly, should we wish to migrate to Rust's newtype idiom then this is already covering half the battle. Should we decide between aliases and newtypes before the `FieldId` step? As I read it, the aliases don't shrink a later newtype migration much. The compiler treats `SchemaId`, `FieldId`, and `i32` as one type, so every expression site keeps compiling under an alias. That covers arithmetic like `get_highest_schema_id() + 1`, comparisons with `LAST_ADDED`, integer literals in tests, and reads of public fields such as [`NestedField::id`](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/spec/datatypes.rs#L618-L620) and [`PartitionField::source_id` and `field_id`](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/spec/partition.rs#L41-L44). Those sites are where a newtype migration costs the most. The alias also doesn't catch mixups. The [`last_added_version_id: Option<SchemaId>`](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg /src/spec/view_metadata_builder.rs#L49) field that #3526 fixes compiled for that reason. Changing a released `pub type FieldId` to `pub struct FieldId` is a break with no deprecation path, because the two can't share a name. Downstream code would change twice. For example, Comet passes `Vec<i32>` from protobuf into [`with_project_field_ids`](https://github.com/apache/datafusion-comet/blob/1ad79380c103f5526628ebaea5de743b42c258c0/native/core/src/execution/planner.rs#L2764) and inserts `field.id` into a set of `i32` ([here](https://github.com/apache/datafusion-comet/blob/1ad79380c103f5526628ebaea5de743b42c258c0/native/core/src/execution/planner.rs#L2623)). If we expect to want newtypes, doing `FieldId` once as a newtype also touches open PRs once instead of twice. For precedent, arrow-rs and DataFusion have no integer type aliases. They use bare integers at interop boundaries, for example [`ProjectionMask::leaves`](https://github.com/apache/arrow-rs/blob/ea3c266746a5108c4df23a34bd51c5e38ba5a4a7/parquet/src/arrow/mod.rs#L312) takes `usize`. Where identity matters they use newtypes with a private field, such as DataFusion's [`SubqueryIndex`](https://github.com/apache/datafusion/blob/aeba33ff9bcd3c6d5424d7025bb8bd7eca1b33b1/datafusion/expr/src/physical_planning_context.rs#L99) and arrow-rs's [`PageKey`](https://github.com/apache/arrow-rs/blob/ea3c266746a5108c4df23a34bd51c5e38ba5a4a7/parquet/src/column/page_store.rs#L43). Could `ArrowFieldIndex` and `ParquetColumnIndex` be `pub(crate)`? They describe reader internals, so keeping them private adds no public API. That also makes them a cheap place for real newtypes. Does `ArrowFieldIndex` mean a top-level field or a leaf? arrow-rs separates the two as [`roots` and `leaves`](https://github.com/apache/arrow-rs/blob/ea3c266746a5108c4df23a34bd51c5e38ba5a4a7/parquet/src/arrow/mod.rs#L312-L325). `ParquetColumnIndex` also reads like Parquet's page index structure [`ColumnIndex`](https://github.com/apache/arrow-rs/blob/ea3c266746a5108c4df23a34bd51c5e38ba5a4a7/parquet/src/file/page_index/column_index.rs#L40). Would a name like `ParquetLeafIndex` avoid both? Should the `-1` sentinel get its own type? [`TableMetadataBuilder::LAST_ADDED`](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/spec/table_metadata_builder.rs#L73) stands in for schema, spec, sort order, and view version IDs. After #3525, [`TableUpdate::SetCurrentSchema`](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/catalog/mod.rs#L525-L528) has `schema_id: SchemaId` holding a value that is not a schema ID. An `Option<SchemaId>` or a small enum, with custom serde for the [REST `-1`](https://github.com/apache/iceberg/blob/30c246a03b43f6f67ebc0e98c5adebef3dbe3c16/open-api/rest-catalog-open-api.yaml#L3194), would make the "last added" case explicit. A newtype forces that decision, and an alias hides it. Should `SortOrderId` go in its own PR labeled breaking? The spec types [`order-id`](https://github.com/apache/iceberg/blob/30c246a03b43f6f67ebc0e98c5adebef3dbe3c16/format/spec.md?plain=1#L2117) as an int, so changing [`SortOrder::order_id: i64`](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/spec/sort.rs#L108) is a spec fix that stands without the alias. Should [`ManifestFile::first_row_id`](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/spec/manifest_list/manifest_file.rs#L105) stay `u64`? The spec initializes [`next-row-id`](https://github.com/apache/iceberg/blob/30c246a03b43f6f67ebc0e98c5adebef3dbe3c16/format/spec.md?plain=1#L605) to 0 and only [adds to it](https://github.com/apache/iceberg/blob/30c246a03b43f6f67ebc0e98c5adebef3dbe3c16/format/spec.md?plain=1#L601), so row IDs are non-negative. The reader already returns an error when assigned IDs would pass `i64::MAX` ([test](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/arrow/manifest.rs#L1244-L1246)). -- 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]
