Skip to content

Add support for unique-table-location#4582

Open
venkateshwaracholan wants to merge 2 commits into
apache:mainfrom
venkateshwaracholan:iceberg-unique-table-location-support
Open

Add support for unique-table-location#4582
venkateshwaracholan wants to merge 2 commits into
apache:mainfrom
venkateshwaracholan:iceberg-unique-table-location-support

Conversation

@venkateshwaracholan
Copy link
Copy Markdown
Contributor

@venkateshwaracholan venkateshwaracholan commented May 29, 2026

Fixes #4492

Summary

Add support for Iceberg's unique-table-location catalog property when creating tables without an explicit location.

Changes

  • Honor CatalogProperties.UNIQUE_TABLE_LOCATION when computing default table locations.
  • Use Iceberg's LocationUtil.tableLocation(...) helper to generate table directory names.
  • Re-enable the previously disabled Iceberg compatibility test createTableInUniqueLocation().
  • Add a changelog entry.

Validation

Verified Iceberg 1.11 uses the same LocationUtil.tableLocation(...) helper when unique-table-location is enabled.

Ran:

./gradlew :polaris-runtime-service:test \
  --tests "org.apache.polaris.service.catalog.iceberg.IcebergCatalogRelationalTest.createTableInUniqueLocation"

Result: passed.

Also ran:

./gradlew :polaris-runtime-service:spotlessJavaCheck
./gradlew compileAll

Both passed.

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

@snazy
Copy link
Copy Markdown
Member

snazy commented Jun 1, 2026

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.

@venkateshwaracholan
Copy link
Copy Markdown
Contributor Author

venkateshwaracholan commented Jun 2, 2026

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.

@snazy 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.

Please take another look when you have a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for unique-table-location property

3 participants