smaheshwar-pltr commented on code in PR #1642:
URL: https://github.com/apache/iceberg-python/pull/1642#discussion_r1954666388
##########
pyiceberg/table/locations.py:
##########
@@ -64,6 +71,35 @@ def new_data_location(self, data_file_name: str,
partition_key: Optional[Partiti
str: A fully-qualified location URI for the data file.
"""
+ def new_table_metadata_file_location(self, new_version: int = 0) -> str:
Review Comment:
This generally looks fine to me (and configuring metadata locations on
`LocationProvider`s is cool)!
1. Spitballing: 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 `LocationProvider`s 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?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
