Skip to content

Fix OPTIMIZED_SIBLING_CHECK ancestor overlap #5520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andybradshaw wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix OPTIMIZED_SIBLING_CHECK ancestor overlap #5520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6442fe5
Add failing test: OPTIMIZED_SIBLING_CHECK rejects entities under thei…
andybradshaw Sep 15, 2026
d79c2ca
Fix OPTIMIZED_SIBLING_CHECK rejecting entities nested under their own…
andybradshaw Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,14 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti

### Fixes

- Fixed `OPTIMIZED_SIBLING_CHECK` rejecting every entity created under a namespace. 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.

- Return HTTP 404 instead of 204 when a generic table or its catalog path disappears after resolution and before deletion.

- Deleting a semantic model now returns HTTP 404 instead of HTTP 500 when the model or its
Expand Down
112 changes: 65 additions & 47 deletions ...etastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@
import org.apache.polaris.persistence.nosql.coretypes.catalog.CatalogRolesObj;
import org.apache.polaris.persistence.nosql.coretypes.catalog.CatalogStateObj;
import org.apache.polaris.persistence.nosql.coretypes.catalog.CatalogsObj;
import org.apache.polaris.persistence.nosql.coretypes.catalog.EntityIdSet;
import org.apache.polaris.persistence.nosql.coretypes.content.ContentObj;
import org.apache.polaris.persistence.nosql.coretypes.mapping.EntityObjMappings;
import org.apache.polaris.persistence.nosql.coretypes.principals.PrincipalObj;
Expand Down Expand Up @@ -630,12 +631,22 @@ <T extends PolarisEntity & LocationBasedEntity> Optional<String> hasOverlappingS
return Optional.empty();
}

var catalogId = entity.getCatalogId();
var checkLocation = StorageLocation.of(baseLocation).withoutScheme();
var entityLocation = StorageLocation.of(baseLocation);

// The entity's own parent namespaces contain its location by construction; they are not
// siblings. Resolve the parent chain up front via the (memoized) id index.
var ancestorIds = new HashSet<Long>();
for (var id = entity.getParentId();
id != PolarisEntityConstants.getNullId() && id != catalogId && ancestorIds.add(id); ) {
var ancestor = lookupEntity(catalogId, id, PolarisEntityType.NAMESPACE.getCode());
if (ancestor == null) {
break;
}
id = ancestor.getParentId();
}

return hasOverlappingSiblings(entity.getCatalogId(), checkLocation);
}

Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
return memoizedIndexedAccess
.catalogContent(catalogId)
.refObj()
Expand All @@ -654,36 +665,61 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
var locationIdentifier = identifierFromLocationString(checkLocation);
var locationIndexKey = locationIdentifier.toIndexKey();

// Resolves an index entry to the base location of the first entity in it that
// actually overlaps the entity being checked.
Function<EntityIdSet, Optional<String>> firstOverlap =
entityIdSet ->
entityIdSet.entityIds().stream()
.map(IndexKey::key)
.map(byId::get)
.filter(Objects::nonNull)
.map(byName::get)
.filter(Objects::nonNull)
.map(objRef -> persistence.fetch(objRef, ContentObj.class))
.filter(Objects::nonNull)
.map(contentObj -> mapToEntity(contentObj, catalogId))
.filter(
candidate -> {
// The entity itself being re-created is an already-exists
// condition for the create, not an overlap.
if (candidate.getParentId() == entity.getParentId()
&& candidate.getType() == entity.getType()
&& candidate.getName().equals(entity.getName())) {
return false;

@dimas-b dimas-b Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +685 to +688

@flyingImer flyingImer Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
var candidateBaseLocation =
candidate.getPropertiesAsMap().get(ENTITY_BASE_LOCATION);
if (candidateBaseLocation == null
|| candidateBaseLocation.isBlank()) {
return false;
}
var candidateLocation = StorageLocation.of(candidateBaseLocation);
var containsEntity = entityLocation.isChildOf(candidateLocation);
var containedByEntity = candidateLocation.isChildOf(entityLocation);
// An ancestor may contain the entity, but the entity may not sit
// at exactly its location.
if (containsEntity
&& !containedByEntity
&& ancestorIds.contains(candidate.getId())) {
return false;
}
return containsEntity || containedByEntity;
})
.map(
candidate -> candidate.getPropertiesAsMap().get(ENTITY_BASE_LOCATION))
.findFirst();

// Check for children and exact matches first using forward iteration (preserves
// existing test expectations on which conflicting location is reported).
// Also iterate fully in case early entries are filtered out.
var iter = locationsIndex.iterator(locationIndexKey, null, false);
while (iter.hasNext()) {
var elem = iter.next();
var elemKey = elem.key();
var elemIdentifier = indexKeyToIdentifier(elemKey);
var elemIdentifier = indexKeyToIdentifier(elem.key());
if (!elemIdentifier.startsWith(locationIdentifier)) {
break; // No more matches due to ordering
}

var conflicting =
elem.value().entityIds().stream()
.map(IndexKey::key)
.map(byId::get)
.filter(Objects::nonNull)
.map(byName::get)
.filter(Objects::nonNull)
.map(objRef -> persistence.fetch(objRef, ContentObj.class))
.filter(Objects::nonNull)
.map(
contentObj -> {
var conflictingBaseLocation =
contentObj.properties().get(ENTITY_BASE_LOCATION);
return conflictingBaseLocation != null
? conflictingBaseLocation
: String.join("/", elemIdentifier.elements());
})
.findFirst();
var conflicting = firstOverlap.apply(elem.value());

@dimas-b dimas-b Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@dimas-b dimas-b Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dimas-b dimas-b Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part apparently comes from #4873.

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

@dimas-b dimas-b Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vigneshio vigneshio Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

@dimas-b dimas-b Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the analysis, @vigneshio !

@dimas-b dimas-b Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dimas-b dimas-b Sep 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// forward iteration starting at the full target key.
for (int i = 1; i <= locationIdentifier.length(); i++) {
var prefixElements = locationIdentifier.elements().subList(0, i);
var prefix = ContentIdentifier.identifier(prefixElements);
var prefixKey = prefix.toIndexKey();
var entry = locationsIndex.get(prefixKey);
var prefix =
ContentIdentifier.identifier(locationIdentifier.elements().subList(0, i));
var entry = locationsIndex.get(prefix.toIndexKey());
if (entry != null) {
var conflicting =
entry.entityIds().stream()
.map(IndexKey::key)
.map(byId::get)
.filter(Objects::nonNull)
.map(byName::get)
.filter(Objects::nonNull)
.map(objRef -> persistence.fetch(objRef, ContentObj.class))
.filter(Objects::nonNull)
.map(
contentObj -> {
var conflictingBaseLocation =
contentObj.properties().get(ENTITY_BASE_LOCATION);
return conflictingBaseLocation != null
? conflictingBaseLocation
: String.join("/", prefix.elements());
})
.findFirst();
var conflicting = firstOverlap.apply(entry);
if (conflicting.isPresent()) {
return conflicting;
}
Expand Down
20 changes: 15 additions & 5 deletions ...c/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,11 +278,7 @@ public void overlappingLocations() {
.contains(Optional.of("s3://bucket/foo/"));

for (var check :
List.of(
"s3://bucket/foo/bar",
"s3://bucket/foo/bar/",
"s3a://bucket/foo/bar/",
"gs://bucket/foo/bar/")) {
List.of("s3://bucket/foo/bar", "s3://bucket/foo/bar/", "s3a://bucket/foo/bar/")) {
soft.assertThat(
metaStore.hasOverlappingSiblings(
callContext,
Expand All @@ -294,6 +290,20 @@ public void overlappingLocations() {
.contains(Optional.of("s3://bucket/foo/bar/"));
}

// The location index is scheme-less, so a same-path location under a different scheme is a
// candidate, but the overlap verdict is scheme-aware (only the S3 family is treated as one
// scheme), consistent with the legacy sibling check. A gs:// path does not overlap an s3://
// one.
soft.assertThat(
metaStore.hasOverlappingSiblings(
callContext,
new NamespaceEntity.Builder(Namespace.of("x"))
.setCatalogId(catalog.getId())
.setBaseLocation("gs://bucket/foo/bar/")
.build()))
.isPresent()
.contains(Optional.empty());

soft.assertThat(
metaStore.hasOverlappingSiblings(
callContext,
Expand Down
74 changes: 56 additions & 18 deletions ...src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.OptionalInt;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Function;
Expand All @@ -45,6 +47,7 @@
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisChangeTrackingVersions;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityConstants;
import org.apache.polaris.core.entity.PolarisEntityCore;
import org.apache.polaris.core.entity.PolarisEntityId;
import org.apache.polaris.core.entity.PolarisEntitySubType;
Expand Down Expand Up @@ -853,24 +856,59 @@ Optional<Optional<String>> hasOverlappingSiblings(
realmId, schemaVersion, entity.getCatalogId(), entity.getBaseLocation());
try {
var results = datasourceOperations.executeSelect(query, new ModelEntity(schemaVersion));
if (!results.isEmpty()) {
StorageLocation entityLocation = StorageLocation.of(entity.getBaseLocation());
for (PolarisBaseEntity result : results) {
// JDBC materializes persisted rows as PolarisBaseEntity. Resolve the sibling location
// via PolarisEntityUtils instead of casting to LocationBasedEntity.
Optional<String> overlappingSiblingLocation =
PolarisEntityUtils.asLocationBasedEntity(PolarisEntity.of(result))
.map(LocationBasedEntity::getBaseLocation)
.filter(location -> location != null && !location.isBlank())
.map(StorageLocation::of)
.filter(
potentialSiblingLocation ->
entityLocation.isChildOf(potentialSiblingLocation)
|| potentialSiblingLocation.isChildOf(entityLocation))
.map(StorageLocation::toString);
if (overlappingSiblingLocation.isPresent()) {
return Optional.of(overlappingSiblingLocation);
}
if (results.isEmpty()) {
return Optional.of(Optional.empty());
}
// The query matches every entity whose location is an ancestor of, equal to, or a descendant
// of the entity's location. The entity's own parent namespaces always match the ancestor
// terms; they are not siblings. Their rows are normally in the result set, so walking the
// parent chain rarely needs an extra lookup.
Map<Long, PolarisBaseEntity> resultsById = new HashMap<>();
results.forEach(result -> resultsById.putIfAbsent(result.getId(), result));
Set<Long> ancestorIds = new HashSet<>();
for (long id = entity.getParentId();
id != PolarisEntityConstants.getNullId()
&& id != entity.getCatalogId()
&& ancestorIds.add(id); ) {
PolarisBaseEntity ancestor = resultsById.get(id);
if (ancestor == null) {
ancestor =
lookupEntity(
callContext, entity.getCatalogId(), id, PolarisEntityType.NAMESPACE.getCode());
}
if (ancestor == null) {
break;
}
id = ancestor.getParentId();
}

StorageLocation entityLocation = StorageLocation.of(entity.getBaseLocation());
for (PolarisBaseEntity result : results) {
// An entity with the same name under the same parent is the entity itself being
// re-created: an already-exists condition for the create, not an overlap.
if (result.getParentId() == entity.getParentId()
&& result.getType() == entity.getType()
&& result.getName().equals(entity.getName())) {
continue;
}
// JDBC materializes persisted rows as PolarisBaseEntity. Resolve the sibling location
// via PolarisEntityUtils instead of casting to LocationBasedEntity.
Optional<StorageLocation> resultLocation =
PolarisEntityUtils.asLocationBasedEntity(PolarisEntity.of(result))
.map(LocationBasedEntity::getBaseLocation)
.filter(location -> location != null && !location.isBlank())
.map(StorageLocation::of);
if (resultLocation.isEmpty()) {
continue;
}
boolean containsEntity = entityLocation.isChildOf(resultLocation.get());
boolean containedByEntity = resultLocation.get().isChildOf(entityLocation);
// An ancestor may contain the entity, but the entity may not sit at exactly its location.
if (containsEntity && !containedByEntity && ancestorIds.contains(result.getId())) {
continue;
}
if (containsEntity || containedByEntity) {
return Optional.of(Optional.of(resultLocation.get().toString()));
}
}
return Optional.of(Optional.empty());
Expand Down
5 changes: 4 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,10 @@ boolean hasChildren(

/**
* Check if the specified IcebergTableLikeEntity / NamespaceEntity has any sibling entities which
* share a base location
* share a base location. The entity's own ancestors are not siblings: a parent namespace whose
* location contains the entity's location is not reported, unless the entity sits at exactly the
* ancestor's location. An existing entity with the same name under the same parent is not
* reported either; that is an already-exists condition for the subsequent create.
*
* @param callContext the polaris call context
* @param entity the entity to check for overlapping siblings for
Expand Down
5 changes: 3 additions & 2 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,8 +443,9 @@ default BaseResult bootstrapPolarisService(@NonNull PolarisCallContext callCtx)
long entityId);

/**
* Check if the specified IcebergTableLikeEntity has any same-namespace siblings which share a
* location
* Check if the specified IcebergTableLikeEntity / NamespaceEntity has any sibling entities which
* share a base location. The entity's own ancestors are not siblings; see {@link
* BasePersistence#hasOverlappingSiblings}.
*
* @param callContext the polaris call context
* @param entity the entity to check for overlapping siblings for
Expand Down
Loading