snazy commented on PR #4582: URL: https://github.com/apache/polaris/pull/4582#issuecomment-4593774768
I don’t think this implementation is correct for Polaris. It appears to satisfy the inherited Iceberg `createTableInUniqueLocation` test, but it does not trace all of Polaris’s create-location paths. The main issue is that `defaultWarehouseLocation(...)` and `buildPrefixedLocation(...)` are shared by more than "direct table create without an explicit location": - Direct table create calls `buildTable(...).withLocation(request.location()).create()`. When `request.location()` is `null`, the Polaris table builder can route through `transformTableLikeLocation(...)`, and depending on prefix config may use `buildPrefixedLocation(...)` or `defaultWarehouseLocation(...)`. - Staged table create without a location uses `buildTable(...).createTransaction().table().location()` in `IcebergCatalogHandler.stageTableCreateHelper(...)`. That does not call `withLocation(null)`, so it bypasses the same transform path used by direct create. This means direct create and staged create can derive different locations under `DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED`. - Views also use the same catalog default-location machinery. `createView(...)` eventually calls `.withLocation(request.location())`, and the Polaris view builder also calls `transformTableLikeLocation(...)`. If `buildPrefixedLocation(...)` or `defaultWarehouseLocation(...)` blindly uses `LocationUtil.tableLocation(...)`, then views can inherit `unique-table-location` and get a `viewName-uuid` suffix, which is not what Iceberg’s table-specific `unique-table-location` property should mean. I think the fix needs a table-specific helper for deriving default table create locations, and both direct create and staged create should use that helper when the request has no explicit location. View default locations should stay view-name-based, and namespace/register/load/drop/rename paths should not be affected. The fix also need targeted tests for those changes. -- 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]
