dennishuo commented on code in PR #1231:
URL: https://github.com/apache/polaris/pull/1231#discussion_r2010998830
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1189,6 +1189,20 @@ private class BasePolarisTableOperations extends
BaseMetastoreTableOperations {
this.tableFileIO = defaultFileIO;
}
+ protected PolarisResolvedPathWrapper getTablePath(TableIdentifier
tableIdentifier) {
Review Comment:
This is actually potentially problematic. Right now, the preemptive "already
exists" checking here in `IcebergCatalog` is actually a bit vestigial and
redundant when the "true" conflict-prevention needs to happen directly in the
persistence layer. Importantly it's only "best-effort" because it doesn't
express the constraint in any way to the conditional write in the persistence
layer.
The current semantic that is designed to be enforced by the persistence
layer is that all entities of the *same* `EntityType` of any `EntitySubType`
must have unique names within a parentId.
Trying to preserve non-conflicting names for GenericTables requires one of
two options:
1. Define a new fundamental persistence constraint on uniqueness of name
given <catalogId, parentId> that only holds a subset of types
(ICEBERG_TABLE_LIKE and GENERIC_TABLE)
2. Go back to using TABLE_LIKE with GENERIC_TABLE subtype
Was the reason for not using TABLE_LIKE just because you didn't want to use
`TableLikeEntity`? The entity wrappers could've been refactored instead of
shifting the EntityTypes -- we could instead push the "iceberg-specific" bits
of `TableLikeEntity` into a new `IcebergTableLikeEntity` and still have a
separate `GenericTableEntity`. Both could happen to use the same TABLE_LIKE
`EntityType`.
--
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]
