mbutrovich commented on code in PR #3057:
URL: https://github.com/apache/iceberg-rust/pull/3057#discussion_r4482999363
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -192,6 +193,86 @@ impl TableMetadataBuilder {
)
}
+ /// Replaces the table's schema, partition spec and sort order, leaving it
with no
+ /// current snapshot. Backs `REPLACE TABLE`.
+ ///
+ /// The table keeps its uuid, properties and snapshots, so earlier
snapshots stay readable.
+ /// Columns the table already has keep their field ids, so data written
before the replacement
+ /// stays readable through the columns both schemas share; new columns get
ids above
+ /// `last_column_id`, never a dropped column's id.
+ ///
+ /// Partition fields reuse the highest id an equivalent field has in any
of the table's specs,
+ /// and other fields get ids above `last_partition_id`.
+ ///
+ /// `location` moves the table, `None` leaves it in place. `properties`
are applied on top of
+ /// the existing ones. `format-version` upgrades the table without being
persisted; other
+ /// reserved properties are ignored.
+ ///
+ /// # Errors
+ /// - The table is format version 1, which is not supported.
+ /// - `format-version` is invalid or would downgrade the table.
+ /// - The partition spec or sort order names a column missing from
`schema`.
+ /// - `schema` takes the name of a partition field in one of the table's
specs without being
+ /// that field's source column.
Review Comment:
Could we add tests for the three errors in this list that no test covers
yet? They are a downgrade through `format-version` (for example a v3 table
given `format-version=2`), a sort order on a column missing from `schema`, and
a replacement schema that adds a column named after a partition field of an
existing spec, such as `x_bucket` on a table partitioned by `bucket(8, x)`. The
existing tests cover the v1, invalid `format-version`, and missing partition
source cases.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -192,6 +193,86 @@ impl TableMetadataBuilder {
)
}
+ /// Replaces the table's schema, partition spec and sort order, leaving it
with no
+ /// current snapshot. Backs `REPLACE TABLE`.
+ ///
+ /// The table keeps its uuid, properties and snapshots, so earlier
snapshots stay readable.
+ /// Columns the table already has keep their field ids, so data written
before the replacement
+ /// stays readable through the columns both schemas share; new columns get
ids above
+ /// `last_column_id`, never a dropped column's id.
+ ///
+ /// Partition fields reuse the highest id an equivalent field has in any
of the table's specs,
+ /// and other fields get ids above `last_partition_id`.
+ ///
+ /// `location` moves the table, `None` leaves it in place. `properties`
are applied on top of
+ /// the existing ones. `format-version` upgrades the table without being
persisted; other
+ /// reserved properties are ignored.
+ ///
+ /// # Errors
+ /// - The table is format version 1, which is not supported.
+ /// - `format-version` is invalid or would downgrade the table.
+ /// - The partition spec or sort order names a column missing from
`schema`.
+ /// - `schema` takes the name of a partition field in one of the table's
specs without being
+ /// that field's source column.
+ pub fn build_replacement(
+ self,
+ schema: Schema,
+ spec: impl Into<UnboundPartitionSpec>,
+ sort_order: SortOrder,
+ location: Option<String>,
+ properties: HashMap<String, String>,
+ ) -> Result<Self> {
+ if self.metadata.format_version == FormatVersion::V1 {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Table replacement is not supported for format version 1",
+ ));
+ }
+
+ let format_version = properties
+ .get(TableProperties::PROPERTY_FORMAT_VERSION)
+ .map(|value| {
+ serde_json::from_str::<FormatVersion>(value).map_err(|err| {
+ invalid_data!("Unsupported format version:
{value}").with_source(err)
+ })
+ })
+ .transpose()?
+ .unwrap_or(self.metadata.format_version);
+ let properties = properties
+ .into_iter()
+ .filter(|(key, _)|
!TableProperties::RESERVED_PROPERTIES.contains(&key.as_str()))
+ .collect();
Review Comment:
Should `build_replacement` take the target version as a typed
`format_version: Option<FormatVersion>`, and pass `properties` to
`set_properties` unchanged?
As I read it, this would be the only entry point in the crate that reads
`format-version` out of a property map.
[`TableMetadataBuilder::new`](https://github.com/apache/iceberg-rust/blob/9bf99975aaca1320111cfe166e583406795087fd/crates/iceberg/src/spec/table_metadata_builder.rs#L80-L87)
takes a typed `FormatVersion`. Both `new` and
[`set_properties`](https://github.com/apache/iceberg-rust/blob/9bf99975aaca1320111cfe166e583406795087fd/crates/iceberg/src/spec/table_metadata_builder.rs#L330-L343)
return an error for any reserved key. So the same property map behaves
differently depending on the entry point. With `uuid` in `properties`, `new`
returns `Table properties should not contain reserved properties, but got:
[uuid]`, while `build_replacement` drops it without an error. A typed argument
would also remove the `serde_json` parse of a string.
Something like:
```rust
pub fn build_replacement(
self,
schema: Schema,
spec: impl Into<UnboundPartitionSpec>,
sort_order: SortOrder,
location: Option<String>,
format_version: Option<FormatVersion>,
properties: HashMap<String, String>,
) -> Result<Self> {
// ...
let format_version =
format_version.unwrap_or(self.metadata.format_version);
```
Once this ships, the stability policy keeps whichever way it handles
reserved keys, so it would be good to settle that in this PR.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -192,6 +193,86 @@ impl TableMetadataBuilder {
)
}
+ /// Replaces the table's schema, partition spec and sort order, leaving it
with no
+ /// current snapshot. Backs `REPLACE TABLE`.
+ ///
+ /// The table keeps its uuid, properties and snapshots, so earlier
snapshots stay readable.
+ /// Columns the table already has keep their field ids, so data written
before the replacement
+ /// stays readable through the columns both schemas share; new columns get
ids above
+ /// `last_column_id`, never a dropped column's id.
+ ///
+ /// Partition fields reuse the highest id an equivalent field has in any
of the table's specs,
+ /// and other fields get ids above `last_partition_id`.
+ ///
+ /// `location` moves the table, `None` leaves it in place. `properties`
are applied on top of
+ /// the existing ones. `format-version` upgrades the table without being
persisted; other
+ /// reserved properties are ignored.
+ ///
+ /// # Errors
+ /// - The table is format version 1, which is not supported.
+ /// - `format-version` is invalid or would downgrade the table.
+ /// - The partition spec or sort order names a column missing from
`schema`.
+ /// - `schema` takes the name of a partition field in one of the table's
specs without being
+ /// that field's source column.
+ pub fn build_replacement(
Review Comment:
Could we add a rustdoc example for `build_replacement` that runs as a
doctest? It's a new public entry point with five positional arguments. An
example that replaces the schema and spec, then calls `build()`, would show
callers how it's meant to be used. It would also show where the caller adds the
snapshot with the new contents before building.
--
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]
