Add table location validation and overlap checking for GenericTable create by gh-yzou · Pull Request #4237 · apache/polaris

gh-yzou · 2026-04-18T00:35:38Z

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)

dimas-b · 2026-05-07T19:20:50Z

@gh-yzou : WDYT about rebasing this PR and opening for full review?

gh-yzou · 2026-05-07T22:16:11Z

@dimas-b sure, there is couple more things i need to fix for this PR, but i will open it for review once it is done

gh-yzou · 2026-05-28T02:13:24Z

+   * base-location property of each. The target entity's base location may not be a prefix or a
+   * suffix of any sibling entity's base location.
+   */
+  public static <T extends PolarisEntity & LocationBasedEntity> void validateNoLocationOverlap(


those functions are pure copy from IcebergCatalog.java

7-arg static method, can we remove the parameter realmConfig as polarisCallContext has it already?

dimas-b

Thanks for the update, @gh-yzou !

I think the concern with the "optimized" location check is pretty serious 🤔

dimas-b · 2026-05-29T17:39:59Z

+  public void testCreateTableWithInvalidLocationFails() {
+    String deltatb = getTableNameWithRandomSuffix();
+    String invalidLocation =
+        new File(System.getProperty("java.io.tmpdir"), "invalid_" + deltatb).getAbsolutePath();


System.getProperty("java.io.tmpdir") looks a bit strange... why not use the @TempDir annotation supported by JUnit?

dimas-b · 2026-05-29T17:42:09Z

+    List<Object[]> joinResult =
+        sql(
+            "SELECT icebergtb.col1 as id, icebergtb.col2 as str_col, deltatb.col2 as int_col from icebergtb inner join deltatb on icebergtb.col1 = deltatb.col1 order by id");
+    assertThat(joinResult.get(0)).isEqualTo(new Object[] {1, "a", 3});


Why does it matter for a "location validation" PR? 🤔

dimas-b · 2026-05-29T17:50:42Z

+                genericTableCatalog.createGenericTable(
+                    TableIdentifier.of("ns", "t2"),
+                    "format",
+                    "s3://my-bucket/path/to/data/ns/t1/sub",


What about the reverse order (first create a Generic table, then an Iceberg table with and overlapping location)?

dimas-b · 2026-05-29T17:58:01Z

+                .setParentId(lastParent.getId())
+                .setBaseLocation(baseLocation)
+                .build();
+        CatalogUtils.validateNoLocationOverlap(


Feedback from artificial helpers:

P1 Generic-table overlap checks are bypassed with OPTIMIZED_SIBLING_CHECK=true. The new generic path calls CatalogUtils.validateNoLocationOverlap
(runtime/service/src/main/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalog.java:128), and CatalogUtils returns immediately when
the optimized check reports no conflict (runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogUtils.java:168). But generic
table locations are stored in internal properties (polaris-core/src/main/java/org/apache/polaris/core/entity/table/GenericTableEntity.java:70), while
optimized location indexing/read paths only include Iceberg tables/views and namespaces (persistence/relational-jdbc/src/main/java/org/apache/polaris/
persistence/relational/jdbc/models/ModelEntity.java:389, polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/
TreeMapTransactionalPersistenceImpl.java:663). With optimized checking enabled, an existing generic table is invisible, so overlapping generic/generic or
Iceberg/generic locations can be accepted despite ALLOW_TABLE_LOCATION_OVERLAP=false.

Would it be possible to run the same tests with and without the "optimized" location check flag?

dimas-b · 2026-05-29T18:00:17Z

-    // Parent of existing location
+    // Generic table at existing iceberg table location
+    assertThat(
+            createGenericTable(


Feedback from artificial helpers:

P3 The new generic-table overlap test helper builds an invalid API request: it sets name and base-location but omits required format (runtime/service/
src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergOverlappingTableTest.java:94). The OpenAPI schema requires both name and format (spec/
polaris-catalog-apis/generic-tables-api.yaml:210), so these tests are not exercising a valid generic-table create request and could fail for the wrong
reason if request validation is tightened.

flyrain

LGTM overall. Thanks for adding it, @gh-yzou !

flyrain · 2026-05-29T23:29:57Z

+          realmConfig,
+          getMetaStoreManager(),
+          getCurrentPolarisContext(),
+          new ResolutionManifestFactoryImpl(


new ResolutionManifestFactoryImpl(diagnostics, callContext.getRealmContext(), resolverFactory) is rebuilt inline at all 3 validateNoLocationOverlap call sites (here + 737 + 1134) from fields that already exist. Hold the factory as a field (like PolarisGenericTableCatalog does) or extract a small helper. Low-risk cleanup, not blocking.

flyrain · 2026-05-29T23:38:24Z

+   * base-location property of each. The target entity's base location may not be a prefix or a
+   * suffix of any sibling entity's base location.
+   */
+  public static <T extends PolarisEntity & LocationBasedEntity> void validateNoLocationOverlap(


7-arg static method, can we remove the parameter realmConfig as polarisCallContext has it already?

flyrain · 2026-05-29T23:40:16Z

              "Failed to fetch resolved parent for TableIdentifier '%s'", tableIdentifier));
    }

+    if (baseLocation != null && !baseLocation.isEmpty()) {


Flagging that this is a behavior change: a generic table created with an explicit overlapping baseLocation that used to succeed now throws ForbiddenException. Should we call it out in CHANGELOG?

snazy · 2026-06-22T11:20:42Z

Heads up: I'm going to delete the branch soon, see dev-mailing-list discussion

github-project-automation Bot added this to Basic Kanban Board Apr 18, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Apr 18, 2026

gh-yzou mentioned this pull request Apr 28, 2026

Credential Vending for Generic Table #4128

Closed

6 tasks

gh-yzou force-pushed the yzou-generic-table-location-validation branch from bebf5c0 to 58095a1 Compare May 23, 2026 00:16

gh-yzou added 4 commits May 26, 2026 13:25

fix tests

fd61947

fix test failure

a8d7ff3

add overlap checking

440a2fe

fix regression tests

88958e7

gh-yzou force-pushed the yzou-generic-table-location-validation branch from f5d3c59 to 88958e7 Compare May 26, 2026 21:51

gh-yzou added 5 commits May 26, 2026 16:56

fix test failure

ab25fdd

add tests

d5845d2

add tests

d10fca5

undo refactor

da68cf1

add integration tests

1b7c5e5

gh-yzou changed the title ~~Add table location validation for GenericTable~~ Add table location validation and overlap checking for GenericTable create May 28, 2026

gh-yzou marked this pull request as ready for review May 28, 2026 01:33

gh-yzou requested review from dimas-b and flyrain May 28, 2026 01:33

gh-yzou marked this pull request as draft May 28, 2026 01:34

add test

62404e7

gh-yzou marked this pull request as ready for review May 28, 2026 02:12

gh-yzou commented May 28, 2026

View reviewed changes

dimas-b reviewed May 29, 2026

View reviewed changes

flyrain reviewed May 29, 2026

View reviewed changes

snazy closed this Jun 24, 2026

snazy deleted the yzou-generic-table-location-validation branch June 24, 2026 08:31

github-project-automation Bot moved this from PRs In Progress to Done in Basic Kanban Board Jun 24, 2026

Uh oh!

Conversation

gh-yzou commented Apr 18, 2026

Checklist

Uh oh!

dimas-b commented May 7, 2026

Uh oh!

gh-yzou commented May 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snazy commented Jun 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants