Add support for write.metadata.path

[geruh]

Adding support for writing metadata to a custom path set via write.metadata.path property. Since the Python library consolidates the table operation classes in both the table and catalog classes, I had to surface the metadata file location handling to the base Catalog class to avoid circular dependencies. This way we are also able to centralize the metadata location handling for table metadata and snapshots.

Relates to #1492

[kevinjqliu]

Thanks for the PR! Generally LGTM. I added some comments around code organization.

Could you also rebase main to fix the conflict?

[kevinjqliu]

generally lgtm, i added a few nit comments.

looks like theres a linter issue too, can you run make lint locally?

[kevinjqliu]

cc @Fokko @smaheshwar-pltr

[kevinjqliu]

LGTM! sorry about the merge conflict again, merging a lot of changes today :P

[smaheshwar-pltr]

This generally looks fine to me (and configuring metadata locations on LocationProviders is cool)!

1. Spitballing (/nits): I wonder if custom location providers that technically didn't subclass LocationProvider but would've worked before still would now fail for metadata writing because they don't have these new methods. If this is the case, it's maybe fine because we can expect them to subclass LocationProvider as per the docs.
2. The way this is written makes me wonder whether users might want to customise their metadata locations under a table instead of providing a hard-coded path - in a similar way to custom LocationProviders do for data files (maybe with mass updates they can have a large number of json/avro files so they could want to do the object storage hashing inside the metadata folder too to reduce throttling?) . I don't see that strong of a use case FWIW, but if I'm wrong could maybe add this to docs about custom location providers or start a larger discussion. Any thoughts folks?

[kevinjqliu]

for 1, we haven't released a version with LocationProvider. so any references going forward should also have the new methods. We probably want to add a note in the location provider docs about the new metadata location feature

The way this is written makes me wonder whether users might want to customise their metadata locations under a table instead of providing a hard-coded path - in a similar way to custom LocationProviders do for data files

does "under a table" mean path relative to the table's base location? The SimpleLocation Provider uses absolute path. The ObjectStoreLocationProvider is special in that it injects some hash after the base location. But i think a custom metadata location provider can do the same.

[smaheshwar-pltr]

Yeah exactly, I was just curious about user demand for a custom metadata location provider. It exceeds the capability of Java custom location providers and posing the question of whether it makes sense for other implementations.

To clarify, no objections from my end. Updating the docs sounds good!

[geruh]

@smaheshwar-pltr Regarding point 1, I had the same question while working on this. Centralizing locations/paths would definitely make future changes easier, especially with the ongoing discussions around relative path work. I found a related issue in the main repo here: apache/iceberg#6809. Which looks like it didn't get any traction but still holds some value.

[kevinjqliu]

oh i see the semantic difference.

new_data_location is an abstract method, which allows custom LocationProviders to override.
load_location_provider returns SimpleLocationProvider by default, which then use the SimpleLocationProvider's new_data_location method in most cases.

we can do the same for new_metadata_location, making it an abstract method so that custom LocationProviders can override. but also provide a default implementation for both SimpleLocationProvider and ObjectStoreLocationProvider

we can then mention in the LocationProvider documentation that new_metadata_location can be overriden

Does this make sense? @smaheshwar-pltr is this what you were pointing out?

[kevinjqliu]

this is interesting because for data file path, we can use TableProperties.WRITE_DATA_PATH to override LocationProvider's self.data_path, which is the prefix of the data file path.
And new_data_location to override the behavior of the fully-qualified data file location.

We're trying recreate the same behavior for metadata path.
TableProperties.WRITE_METADATA_PATH to override the LocationProvider's self.metadata_path, which is the prefix of the metadata file path.
And new_metadata_location to override the behavior of the fully-qualified metadata file location.

[kevinjqliu]

ok, brushing up on my python...
i think the code is fine as is. both new_metadata_location and new_table_metadata_file_location can provide default implementation and also be overriden by custom implementations

[smaheshwar-pltr]

I found a related issue in the main repo here: apache/iceberg#6809. Which looks like it didn't get any traction but still holds some value.

Thanks for digging into this - I wasn't aware of that discussion. Quoting from it, "Object storage mode to not just data files but also metadata files" is what I was getting at here. It's interesting

[kevinjqliu]

LGTM! Thanks for refactoring the code

I think we should add a comment to the Location Provider documentation and mention the metadata path

[kevinjqliu]

for 1, we haven't released a version with LocationProvider. so any references going forward should also have the new methods. We probably want to add a note in the location provider docs about the new metadata location feature

The way this is written makes me wonder whether users might want to customise their metadata locations under a table instead of providing a hard-coded path - in a similar way to custom LocationProviders do for data files

does "under a table" mean path relative to the table's base location? The SimpleLocation Provider uses absolute path. The ObjectStoreLocationProvider is special in that it injects some hash after the base location. But i think a custom metadata location provider can do the same.

[kevinjqliu]

LGTM! I'll wait for fokko and smaheshwar to chime in :)

[Fokko]

This looks good to me, thanks @geruh for working on this! 🙌

[smaheshwar-pltr]

LGTM (Tiny docs nits)

Thank you @geruh for the location provider changes!

[kevinjqliu]

Thanks for the great suggestions, i added them and double check the docs rendering

[kevinjqliu]

Thanks @geruh for the contribution! And thanks @smaheshwar-pltr @Fokko for the review!