andybradshaw commented on code in PR #2776:
URL: https://github.com/apache/iceberg-rust/pull/2776#discussion_r3554330589
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -47,6 +47,11 @@ use crate::{Error, ErrorKind};
static MAIN_BRANCH: &str = "main";
pub(crate) static ONE_MINUTE_MS: i64 = 60_000;
+/// Table property for the base location of metadata files.
+const WRITE_METADATA_LOCATION: &str = "write.metadata.path";
Review Comment:
nit: It feels like this could be moved to `TableProperties`?
##########
crates/iceberg/src/catalog/metadata_location.rs:
##########
@@ -50,7 +53,7 @@ impl MetadataLocation {
)]
pub fn new_with_table_location(table_location: impl ToString) -> Self {
Self {
- table_location: table_location.to_string(),
+ metadata_dir: format!("{}/metadata", table_location.to_string()),
Review Comment:
Thoughts on handling this formatting within a function in TableMetadata:
```rust
/// Default metadata directory (`<table_location>/metadata`) used when
/// `write.metadata.path` is not configured.
pub(crate) fn default_metadata_dir(table_location: &str) -> String {
format!("{table_location}/{DEFAULT_METADATA_DIR}")
}
```
That way we don't have two `"metadata"` strings floating around?
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -2442,12 +2442,13 @@ mod tests {
"v2"
);
- // metadata version should be bumped
+ // metadata version should be bumped and the new metadata file is
written under the updated
+ // table location's metadata directory
assert!(
updated_table
.metadata_location()
.unwrap()
- .starts_with("s3://bucket/test/location/metadata/00001-")
+
.starts_with("s3://bucket/test/new_location/data/metadata/00001-")
Review Comment:
This feels distinctly different than just respecting the table property...
should this be a fix on it's own to have `MetadataLocation::with_new_metadata`
respect location updates?
--
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]
