venkateshwaracholan commented on PR #4582: URL: https://github.com/apache/polaris/pull/4582#issuecomment-4601163321
> 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. Thanks for the detailed review — it helped highlight the Polaris-specific create-location paths that weren't covered by the Iceberg compatibility test. I've pushed an update that addresses the staged create and view-location concerns, and added targeted tests for both scenarios. I also updated the location derivation logic so direct and staged table creates follow the same path when unique-table-location is enabled. -- 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]
