Fix OPTIMIZED_SIBLING_CHECK ancestor overlap by andybradshaw · Pull Request #5520 · apache/polaris

andybradshaw · 2026-09-15T14:42:03Z

The location index lookup returns the new entity's own parent namespaces (their locations contain the new location by construction), and each backend treated them as overlapping siblings, so nested namespace creation and default-location table creation failed with 403 Forbidden, and re-creating an existing namespace returned 403 instead of 409. The JDBC, NoSQL, and in-memory implementations of hasOverlappingSiblings now exclude the entity's ancestors (when they strictly contain it) and the entity itself before reporting an overlap, matching the legacy sibling check.

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)

…r own ancestors With OPTIMIZED_SIBLING_CHECK enabled, creating a namespace under another namespace at its default location is rejected with a ForbiddenException naming the parent namespace as the conflicting entity. The same happens for a table created at its default location under a namespace. hasOverlappingSiblings finds every entity in the catalog whose location is a prefix of, or lies under, the new location, then flags any that contain or are contained by it. The new entity's own parent chain always satisfies the first condition and is never excluded, so every nested create fails. The legacy path only compared against entities sharing the same parent, so the parent itself was never a candidate. All three backends (relational JDBC, NoSQL, in-memory TreeMap) exhibit the failure.

… ancestors The location-index lookup behind OPTIMIZED_SIBLING_CHECK returns every entity whose base location is an ancestor of, equal to, or a descendant of the new entity's location. That set always includes the new entity's own parent namespaces, and each backend reported the first such row as an overlap. As a result every nested namespace create and every default-location table create under a namespace failed with 403, and re-creating an existing namespace returned 403 instead of 409. This fixes each persistence implementation, using what that backend already knows. JDBC resolves the parent chain from the rows the overlap query returned, falling back to lookupEntity for gaps. The TreeMap store resolves it from the full scan it already performs. NoSQL resolves it through its memoized id index. Each then ignores an ancestor that strictly contains the target and an entity with the same name under the same parent before applying the existing two-way isChildOf test. The NoSQL unit test previously expected gs://bucket/foo/bar/ to overlap an existing s3://bucket/foo/bar/ because that backend compared scheme-less index keys. The verdict is now scheme-aware like the legacy sibling check and the JDBC filter, so that expectation is inverted with a comment.

dimas-b

Nice catch, @andybradshaw ! I cannot imagine how this issue was not noticed before 🤦

Still, I have some questions about specific code changes.

dimas-b · 2026-09-16T00:14:47Z

+                                if (candidate.getParentId() == entity.getParentId()
+                                    && candidate.getType() == entity.getType()
+                                    && candidate.getName().equals(entity.getName())) {
+                                  return false;


Do we have a test for this case? (I might have missed it 😅 )

dimas-b · 2026-09-16T00:26:00Z

-                                  : String.join("/", elemIdentifier.elements());
-                            })
-                        .findFirst();
+                var conflicting = firstOverlap.apply(elem.value());


The old code for this case does not break the new test : testDefaultLocationsUnderAncestorsAreNotOverlaps ... Why do we have to alter it?

dimas-b · 2026-09-16T00:44:42Z

@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
              // Check for parent (prefix) overlaps. These have shorter keys and are missed by


Per method's javadoc, it looks like only siblings need to be checked... why do we recurse into all parents? 🤔

This part apparently comes from #4873.

@vigneshio : Could you recap why this logic was needed?

It looks like the behaviour of hasOverlappingSiblings() has evolved to include any location overlap with any entity... hence #5521 🤔

This part apparently comes from #4873.

@vigneshio : Could you recap why this logic was needed?

@dimas-b the prefix loop isn't walking entity.parentId. #4873 closed a NoSQL index hole so the optimized check matched JDBC.

#5520's ancestorIds walk (entity.getParentId() … catalog) is the opposite - that's how we skip own parents after the index hits, not how we find overlaps.

#1686 introduced the API and the "siblings" name, but the query was already catalog-scoped:

WHERE realm_id = ? AND catalog_id = ? AND (<prefix equality terms> OR <child LIKE> )

The leftover // realmId and parentId go first comment sits above a catalogId bind. So the javadoc has never described the optimized path (BasePersistence: "sibling entities which share a base location"; PolarisMetaStoreManager: "same-namespace siblings"), and the JDBC result set has included the new entity's own ancestors since #1686, with nothing excluding them.

In-memory does the same full scan with isChildOf both ways (#1966); still no ancestor skip. On Postgres/Cockroach, idx_locations led with parent_id until #5301 realigned it to catalog_id. H2 already indexed catalog_id.

That's the mismatch behind #5521, and it predates #4873. The flag-off path is still the same-parent list in validateNoLocationOverlap - that's not this query.

What #4873 fixed is narrower. NoSQL keys are path components, and the iterator started at the full target key, so shorter containing keys sorted before the start and were never visited.

The regression is a foreign occupant on a parent path, not "my own parent namespace":

existing NAMESPACE ns2 @ s3://bucket/foo/

probe Namespace.of("x") @ s3://bucket/foo/newchild/

expect overlap with s3://bucket/foo/

x is not a child of ns2. JDBC caught that; NoSQL silently didn't. #4873 added the prefix lookups plus full-range iteration, and LocalIcebergCatalogNoSqlOverlapTest after IcebergOverlappingTableTest (in-memory) didn't cover NoSQL.

Side effect: before #4873, NoSQL also never saw its own parent, so nested default-location creates passed there by accident. Afterwards it found the parent and reported it - same as JDBC and in-memory already did. That's why #5521 reproduces on all three (catalog-root creates escape because catalogs aren't in the location index).

#5520 draws the right line:

keep searching location prefixes - a foreign occupant on a parent path must still conflict; dropping that reopens Fix correctness gap in NoSQL hasOverlappingSiblings for parent locations #4873

don't treat the entity's own ancestor chain as a conflict when it only strictly contains the new location

don't treat same parentId + type + name as an overlap (create retry should be 409, not 403)

One thing I noticed is I don't see a direct test for that last skip. A locked pair would cover it: foreign prefix still overlaps; own parent on a default child location does not; re-create is 409 not 403. Javadoc for catalog-wide containment would help; renaming the method isn't needed to land this.

Thanks for the analysis, @vigneshio !

So going to the caller code - validateNoLocationOverlap() - if OPTIMIZED_SIBLING_CHECK is off, that method only checks true siblings within the same namespace.

polaris/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java

Line 1527 in 3bdf7da

private <T extends PolarisEntity & LocationBasedEntity> void validateNoLocationOverlap(

However, if OPTIMIZED_SIBLING_CHECK is on, the check is across all entities within the catalog.

This is a logical inconsistency, IMHO, because the flag indicates an optimization, so the behaviour should remain the same with or without the flag (plus or minus performance effects).

I'll open a dev discussion for this.

Cf. https://lists.apache.org/thread/m0jy5mh7wch0wxm5gl2q52g9g4s6krr8

andybradshaw · 2026-09-16T14:22:24Z

@dimas-b thanks for the comments and questions! Will try to respond today

dimas-b · 2026-09-16T14:24:22Z

@andybradshaw : no rush... TBH, I think we're into some substantial cleanup here 😅

flyingImer

One correctness issue remains, details inline

flyingImer · 2026-09-16T21:19:34Z

+                                if (candidate.getParentId() == entity.getParentId()
+                                    && candidate.getType() == entity.getType()
+                                    && candidate.getName().equals(entity.getName())) {
+                                  return false;


The new test doesn't exercise this skip, iirc.

Could we cover recreating a non-empty namespace as well? In #5521, parent.child and parent.t are created before parent is recreated. This filter skips parent itself, but its children still count as overlaps, so createNamespaceInternal throws 403 before reaching the create operation that would return 409. This affects all three implementations I believe. We should cover both empty and non-empty namespace recreation returning AlreadyExists, while retaining overlap rejection for genuinely new namespaces.

flyingImer · 2026-09-16T21:20:14Z

+    TableIdentifier tableAtNamespaceLocation =
+        TableIdentifier.of(grandchild, "table-at-namespace-location");
+    String grandchildLocation = STORAGE_LOCATION + "/overlap-ancestor-parent/child/grandchild";
+    assertThatThrownBy(


nit: could we check the equal-ancestor case before creating the default-location table? That table already sits below grandchildLocation, so it independently causes this create to fail. The assertion can therefore pass even if the namespace at exactly that location is incorrectly skipped. Running it while the namespace is empty would isolate the rule this part of the shared test is intended to protect.

andybradshaw added 2 commits September 15, 2026 10:32

github-project-automation Bot added this to Basic Kanban Board Sep 15, 2026

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

dimas-b reviewed Sep 16, 2026

View reviewed changes

dimas-b requested a review from snazy September 16, 2026 00:44

dimas-b mentioned this pull request Sep 16, 2026

OPTIMIZED_SIBLING_CHECK rejects every namespace and table created under a namespace #5521

Open

flyingImer reviewed Sep 16, 2026

View reviewed changes

		@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
		// Check for parent (prefix) overlaps. These have shorter keys and are missed by

Conversation

andybradshaw commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

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!

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!

andybradshaw commented Sep 16, 2026

Uh oh!

dimas-b commented Sep 16, 2026

Uh oh!

flyingImer 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andybradshaw commented Sep 15, 2026 •

edited

Loading