mkroll-db commented on code in PR #3055:
URL: https://github.com/apache/iceberg-rust/pull/3055#discussion_r4069612985
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -1133,6 +1134,22 @@ impl SessionCatalog for RestSessionCatalog {
let table_ident = TableIdent::new(namespace.clone(),
creation.name.clone());
+ let mut properties = creation.properties;
+
+ if properties.contains_key(TableProperties::PROPERTY_FORMAT_VERSION) {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Table properties should not contain reserved properties,
but got: [{}]. Set `TableCreation::format_version` instead",
+ TableProperties::PROPERTY_FORMAT_VERSION
+ ),
+ ));
+ }
Review Comment:
What about following order:
1. Explicit TableCreation::format_version`
2. `properties["format-version"]`
3. Otherwise omit it (taken from java implemtation)
If it does not match it's just ignored, alternative would be to throw an
error as suggested.
I also took a look at the java implementation. It uses following order to
determine the properties:
1. Use catalog overrides
2. Use caller defaults (only via properties as there is no explicit call)
3. Use catalog defaults
Is there a reason, why the catalog defaults and overrides are not applied in
rust?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -1133,6 +1134,22 @@ impl SessionCatalog for RestSessionCatalog {
let table_ident = TableIdent::new(namespace.clone(),
creation.name.clone());
+ let mut properties = creation.properties;
+
+ if properties.contains_key(TableProperties::PROPERTY_FORMAT_VERSION) {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Table properties should not contain reserved properties,
but got: [{}]. Set `TableCreation::format_version` instead",
+ TableProperties::PROPERTY_FORMAT_VERSION
+ ),
+ ));
+ }
+ properties.insert(
+ TableProperties::PROPERTY_FORMAT_VERSION.to_string(),
+ (creation.format_version as u8).to_string(),
+ );
Review Comment:
I took a look at the java implementation and the default is to omit the
`format_version` and have the catalog pick it.
Is it on purpose that rust defaults to V2?
If not I'd suggest:
```suggestion
if let Some(format_version) = creation.format_version {
properties.insert(
TableProperties::PROPERTY_FORMAT_VERSION.to_string(),
(format_version as u8).to_string(),
);
}
```
Plus
```diff
diff --git a/crates/iceberg/src/catalog/mod.rs
b/crates/iceberg/src/catalog/mod.rs
index d2ecc7d51..e5021ea65 100644
--- a/crates/iceberg/src/catalog/mod.rs
+++ b/crates/iceberg/src/catalog/mod.rs
@@ -362,8 +362,8 @@ pub struct TableCreation {
}))]
pub properties: HashMap<String, String>,
/// Format version of the table. Defaults to V2.
- #[builder(default = FormatVersion::V2)]
- pub format_version: FormatVersion,
+ #[builder(default, setter(strip_option))]
+ pub format_version: Option<FormatVersion>,
}
/// TableCommit represents the commit of a table in the catalog.
```
--
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]
