rdblue commented on a change in pull request #2855:
URL: https://github.com/apache/iceberg/pull/2855#discussion_r687083141
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -154,6 +155,25 @@ public String toString() {
protected abstract String defaultWarehouseLocation(TableIdentifier
tableIdentifier);
+ protected TableLocationSupplier tableLocationSupplier(TableIdentifier
tableIdentifier) {
Review comment:
Why did you choose to make this a supplier rather than passing the
incoming options to a method like `defaultWarehouseLocation`? Is that because
the table UUID is created in the `TableMetadata` constructor?
Are there cases where you think the location would be based on the schema or
sort order? I can't think of valid cases and I wonder if this could actually be
a bad idea. For example, maybe this could look for a column `pii struct<email
string, ...>` and choose to place the table in a "sensitive" bucket, but I
don't think that would be a good pattern for people to follow because it is
error-prone: using `PII` for the column name could break it.
I'm also skeptical that location would be based on the partition spec,
especially because the spec may change.
--
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]
