feat: Honor write.metadata.path for metadata file locations by zakariya-s · Pull Request #2776 · apache/iceberg-rust

zakariya-s · 2026-07-06T18:39:04Z

Which issue does this PR close?

Closes Honor write.metadata.path table property for metadata file locations #2775.

What changes are included in this PR?

Metadata files were always written under <table-location>/metadata, ignoring the write.metadata.path table property. Iceberg Java honors it (via BaseMetastoreTableOperations.metadataFileLocation); this brings iceberg-rust in line and mirrors the existing write.data.path handling for data files.

Add TableMetadata::metadata_location_root(), returning write.metadata.path (trailing slash trimmed) when set, otherwise <location>/metadata.
Use it in SnapshotProducer for the manifest and manifest-list paths, and in MetadataLocation for the table metadata JSON.
MetadataLocation now stores the resolved metadata directory rather than the table location. On commit, the directory is re-derived from the metadata being committed (the version is still parsed from the previous file name), matching Iceberg Java. As a result MetadataLocation::from_str no longer requires the file to live under a /metadata directory (a custom write.metadata.path does not have to); file-name validation is unchanged.

Are these changes tested?

Unit tests:

TableMetadata::metadata_location_root(): default, configured write.metadata.path, and trailing-slash trimming.
MetadataLocation: create (new_with_metadata) and commit (with_new_metadata) honor write.metadata.path; from_str round-trip cases updated for arbitrary metadata dirs.
A fast_append transaction test asserting the manifest list and manifests are written under a configured write.metadata.path.
Updated catalog::tests::test_table_commit to reflect the new metadata file following the updated table location.

Metadata files (manifest lists, manifests, and the table metadata JSON) were always written under `<table-location>/metadata`, ignoring the `write.metadata.path` table property. iceberg-java honors it via `metadataFileLocation`; this brings iceberg-rust in line, mirroring how `write.data.path` is already honored for data files. Add `TableMetadata::metadata_location_root()`, returning `write.metadata.path` (trailing slash trimmed) when set, otherwise `<location>/metadata`. Use it in `SnapshotProducer` for the manifest and manifest-list paths, and in `MetadataLocation` for the metadata JSON. `MetadataLocation` now stores the resolved metadata directory rather than the table location. On commit the directory is re-derived from the metadata being committed (the version is still parsed from the previous file name), matching iceberg-java. As a result `MetadataLocation::from_str` no longer requires the file to live under a `/metadata` directory; file-name validation is unchanged.

…efault metadata dir

zakariya-s · 2026-07-09T22:09:01Z

Thank you for the review @andybradshaw! I think I've addressed them so far, please do let me know if there are any other flags you can see

andybradshaw

Seems fine to me, but will need to get another review/approval from a maintainer.

xanderbailey

Thanks for the PR, just one comment I think

zakariya-s · 2026-07-15T12:50:08Z

Thanks @xanderbailey! I think I've addressed the comment. I did notice though that write.data.path isn't fetched from TableProperties:

iceberg-rust/crates/iceberg/src/writer/file_writer/location_generator.rs

Lines 60 to 62 in b80ebbf

    
           let configured_data_location = prop 
        
               .get(WRITE_DATA_LOCATION) 
        
               .or(prop.get(WRITE_FOLDER_STORAGE_LOCATION));

Might create an issue for this later if you think it's worth it

xanderbailey · 2026-07-17T21:04:47Z

It's a new(ish) struct in the repo so we haven't moved everything there yet but I think typically having it strongly typed is preferable.

CTTY

Thanks for the contribution! Just took a quick look, the logic looks correct but feels like we are overcomplicating this a bit: The core logic is "use write.metadata.path if set, else {table_location}/metadata", which fits in a single function metadata_location() in table_metadata.rs

- collapse the three metadata-dir helpers into a single TableMetadata::metadata_location(), mirroring the encryption-key-id style of an optional strongly-typed property - parse write.metadata.path inline in TableProperties (typed field), dropping PROPERTY_WRITE_METADATA_PATH_DEFAULT_DIR - MetadataLocation::new_with_metadata defaults under the passed table location; drop the redundant new_with_table_location constructor

zakariya-s · 2026-07-18T18:30:59Z

Thanks for the review @CTTY! I've gone over the comments, would really appreciate another review when you have time!

CTTY

Just took another pass and it looks much better! I have left some questions

CTTY · 2026-07-18T22:13:04Z

    pub fn with_new_metadata(&self, new_metadata: &TableMetadata) -> Self {
        Self {
-            table_location: self.table_location.clone(),
+            location: self.location.clone(),


Should we honor the metadata path here as well? What's the expected behavior when new_metadata has a new metadata path?

I haven't checked how it's like in java tho

CTTY · 2026-07-18T22:14:12Z

+        let table_location = table_location.to_string();
+        let location = metadata
+            .table_properties()
+            .ok()


We should throw error here, let's change the current method to try_new_with_metadata if needed

we can also reuse the table_metadata::metadata_location() after refactoring

andybradshaw reviewed Jul 9, 2026

View reviewed changes

Comment thread crates/iceberg/src/spec/table_metadata.rs Outdated

Comment thread crates/iceberg/src/catalog/metadata_location.rs Outdated

Comment thread crates/iceberg/src/catalog/mod.rs Outdated

zakariya-s added 2 commits July 9, 2026 14:49

address review: move write.metadata.path to TableProperties; dedupe d…

840b992

…efault metadata dir

revert metadata directory re-derivation on commit

9b276d0

zakariya-s requested a review from andybradshaw July 9, 2026 22:10

andybradshaw approved these changes Jul 10, 2026

View reviewed changes

xanderbailey reviewed Jul 13, 2026

View reviewed changes

Comment thread crates/iceberg/src/spec/table_metadata.rs Outdated

xanderbailey reviewed Jul 13, 2026

View reviewed changes

Comment thread crates/iceberg/src/catalog/metadata_location.rs

address review: surface write.metadata.path on TableProperties

73e4db9

zakariya-s requested a review from xanderbailey July 15, 2026 12:44

CTTY reviewed Jul 18, 2026

View reviewed changes

Comment thread crates/iceberg/src/catalog/metadata_location.rs Outdated

Comment thread crates/iceberg/src/spec/table_properties.rs Outdated

Comment thread crates/iceberg/src/catalog/metadata_location.rs Outdated

Comment thread crates/iceberg/src/spec/table_metadata.rs Outdated

zakariya-s requested a review from CTTY July 18, 2026 18:31

CTTY reviewed Jul 18, 2026

View reviewed changes

Uh oh!

Conversation

zakariya-s commented Jul 6, 2026

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zakariya-s commented Jul 9, 2026

Uh oh!

andybradshaw left a comment

Choose a reason for hiding this comment

Uh oh!

xanderbailey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zakariya-s commented Jul 15, 2026

Uh oh!

xanderbailey commented Jul 17, 2026

Uh oh!

CTTY left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zakariya-s commented Jul 18, 2026

Uh oh!

CTTY left a comment

Choose a reason for hiding this comment

Uh oh!

CTTY Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

CTTY Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

CTTY Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants