CTTY commented on code in PR #2776:
URL: https://github.com/apache/iceberg-rust/pull/2776#discussion_r3607082887
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -228,6 +231,14 @@ impl TableProperties {
/// Default target file size
pub const PROPERTY_WRITE_TARGET_FILE_SIZE_BYTES_DEFAULT: usize = 512 *
1024 * 1024; // 512 MB
+ /// Base location for metadata files (manifests, manifest lists, table
metadata).
+ /// When unset, metadata files default to the `metadata` directory under
the table
+ /// location.
+ pub const PROPERTY_WRITE_METADATA_PATH: &str = "write.metadata.path";
+ /// Default subdirectory (under the table location) for metadata files when
+ /// [`Self::PROPERTY_WRITE_METADATA_PATH`] is not configured.
+ pub const PROPERTY_WRITE_METADATA_PATH_DEFAULT_DIR: &str = "metadata";
Review Comment:
We can just call it `METADATA_FOLDER_NAME`. it's not a table property, and
we should move this to `metadata_location.rs`
##########
crates/iceberg/src/catalog/metadata_location.rs:
##########
@@ -25,11 +25,14 @@ use crate::compression::CompressionCodec;
use crate::spec::{TableMetadata, parse_metadata_file_compression};
use crate::{Error, ErrorKind, Result};
-/// Helper for parsing a location of the format:
`<location>/metadata/<version>-<uuid>.metadata.json`
-/// or with compression:
`<location>/metadata/<version>-<uuid>.gz.metadata.json`
+/// Helper for parsing a location of the format:
`<metadata-dir>/<version>-<uuid>.metadata.json`
+/// or with compression: `<metadata-dir>/<version>-<uuid>.gz.metadata.json`
+///
+/// `<metadata-dir>` is set to the `write.metadata.path` table property and
+/// it defaults to the `<location>/metadata` when the property is not set.
#[derive(Clone, Debug, PartialEq)]
pub struct MetadataLocation {
- table_location: String,
+ metadata_dir: String,
Review Comment:
nit: how about just `location` or `medata_location` so the actual file lives
under `{location}/actual_files`? it matches with the config so we have less
jargons to play around
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -364,6 +364,33 @@ impl TableMetadata {
&self.properties
}
+ /// Returns the default metadata directory for a table location: the
`metadata`
+ /// subdirectory under `table_location`. Used when `write.metadata.path`
is not set.
+ pub fn default_metadata_dir(table_location: &str) -> String {
Review Comment:
I think we can just call format whenever we need to use it, this feels
excessive
##########
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:
TableMetadata::default_metadata_dir(&table_location.to_string()),
Review Comment:
I think it's a good time to remove this api, it's been deprecated for a while
--
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]
