You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "andybradshaw (via GitHub)" <gi...@apache.org> on 2026/09/15 14:42:06 UTC
[PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
andybradshaw opened a new pull request, #5520:
URL: https://github.com/apache/polaris/pull/5520
Fixes `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.
## Checklist
- [X] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [X] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [X] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [X] ๐ก Added comments for complex logic
- [X] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on PR #5520:
URL: https://github.com/apache/polaris/pull/5520#issuecomment-5699063202
@dimas-b thanks for the comments and questions! Will try to respond today
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4085697892
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java:
##########
@@ -397,18 +397,33 @@ boolean hasChildren(
long parentId);
/**
- * Check if the specified IcebergTableLikeEntity / NamespaceEntity has any sibling entities which
- * share a base location
+ * Check whether the base location of the specified IcebergTableLikeEntity / NamespaceEntity
+ * overlaps the base location of another entity in the same catalog.
+ *
+ * <p>Despite the name, implementations do not limit the search to siblings under the same parent.
+ * They consider every table, view, and namespace in the catalog whose base location contains,
+ * equals, or is contained by the entity's base location. This is the check behind {@code
+ * OPTIMIZED_SIBLING_CHECK}; the fallback used when that flag is off inspects only the entities
+ * directly under the same parent.
+ *
+ * <p>The entity's own ancestors, given by {@code catalogPath}, are excluded: a parent namespace
+ * whose location strictly contains the entity's location is not reported, since default locations
+ * nest under the parent's location. An ancestor whose location equals the entity's location is
+ * still reported.
*
* @param callContext the polaris call context
- * @param entity the entity to check for overlapping siblings for
- * @return Optional.of(Optional.of(location)) if the parent entity has children,
- * Optional.of(Optional.empty()) if not, and Optional.empty() if the metastore doesn't support
- * this operation
+ * @param catalogPath the resolved path of the entity's ancestors: the catalog first, then each
+ * parent namespace
+ * @param entity the entity whose base location to check
+ * @return Optional.of(Optional.of(location)) with the base location of a conflicting entity,
+ * Optional.of(Optional.empty()) if there is no conflict, and Optional.empty() if the
+ * metastore doesn't support this operation
*/
default <T extends PolarisEntity & LocationBasedEntity>
Optional<Optional<String>> hasOverlappingSiblings(
- @NonNull PolarisCallContext callContext, T entity) {
+ @NonNull PolarisCallContext callContext,
+ @NonNull List<PolarisEntityCore> catalogPath,
Review Comment:
Updated... yeah, that makes sense
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4085250312
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -630,12 +631,26 @@ <T extends PolarisEntity & LocationBasedEntity> Optional<String> hasOverlappingS
return Optional.empty();
}
+ var catalogId = entity.getCatalogId();
var checkLocation = StorageLocation.of(baseLocation).withoutScheme();
- return hasOverlappingSiblings(entity.getCatalogId(), checkLocation);
+ // The entity's own parent namespaces contain its location by construction; they are not
Review Comment:
Yep, that makes sense... have updated the comments here (and in the other persistence impls) to reflect that.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #5520:
URL: https://github.com/apache/polaris/pull/5520
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4085274369
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -853,24 +856,52 @@ 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(
Review Comment:
Yeah, this was my original thought when looking to fix this, but wasn't sure how folks would respond to updating the persistence interface... ultimately, it's only one more argument and does clean things up a good bit, so am in favor of using `parentPath` as authoritative.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4065854072
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -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;
+ }
+ 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());
Review Comment:
Reverted this, although I believe what this was modifying should probably be fixed in a follow-up (overlap checks should be scheme-aware, so different backing object stores don't unnecessarily conflict).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4085367424
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -853,24 +856,52 @@ 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(
Review Comment:
SPI changes are ok per our [Evolution Policy](https://polaris.apache.org/releases/1.7.0/evolution/).
In this case the functionality is extended. Implementations are free to ignore the new parameter.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4021510804
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
// Check for parent (prefix) overlaps. These have shorter keys and are missed by
Review Comment:
Per method's javadoc, it looks like only siblings need to be checked... why do we recurse into all parents? ๐ค
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4106264653
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -688,6 +688,18 @@ private void createNamespaceInternal(
.setCreateTimestamp(System.currentTimeMillis())
.setBaseLocation(baseLocation)
.build();
+ EntityResult existingNamespace =
+ getMetaStoreManager()
+ .readEntityByName(
+ getCurrentPolarisContext(),
+ PolarisEntity.toCoreList(resolvedParent.getRawFullPath()),
+ PolarisEntityType.NAMESPACE,
+ PolarisEntitySubType.ANY_SUBTYPE,
+ entity.getName());
+ if (existingNamespace.isSuccess()) {
+ throw new AlreadyExistsException(
Review Comment:
Resolving given https://github.com/apache/polaris/pull/5596 has merged
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4026720287
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
// Check for parent (prefix) overlaps. These have shorter keys and are missed by
Review Comment:
> 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 #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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4065825139
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -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;
Review Comment:
I believe this is now covered in the tests on [this commit](https://github.com/apache/polaris/pull/5520/changes/35e0530e72f9c225c9b1c2ed735f3d2be551ead1#diff-e001b92e83f8817ddd60124eea856808c9e2fdf96923ca1e00a9b16794ef51e4R346-R369), although the fact that it affected every implementation felt like performing the "already exists" check before performing any of the overlap conflict checks was the better approach. Let me know if you disagree!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4065880613
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java:
##########
@@ -295,4 +297,48 @@ public void testParentChildLocationOverlapWithOptimizedSiblingCheck() {
.hasMessageContaining("Unable to create entity at location")
.hasMessageContaining("conflicts with existing table or namespace");
}
+
+ @Test
+ public void testDefaultLocationsUnderAncestorsAreNotOverlaps() {
+ // A namespace's default location is nested under its parent's location, and a table's default
+ // location is nested under its namespace's location. Those ancestors are not siblings and must
+ // never be reported as overlaps, regardless of which sibling-check implementation is active.
+ Namespace parent = Namespace.of("overlap-ancestor-parent");
+ catalog().createNamespace(parent);
+
+ Namespace child = Namespace.of("overlap-ancestor-parent", "child");
+ assertThatCode(() -> catalog().createNamespace(child)).doesNotThrowAnyException();
+
+ Namespace grandchild = Namespace.of("overlap-ancestor-parent", "child", "grandchild");
+ assertThatCode(() -> catalog().createNamespace(grandchild)).doesNotThrowAnyException();
+
+ TableIdentifier table = TableIdentifier.of(grandchild, "table-at-default-location");
+ assertThatCode(() -> catalog().buildTable(table, SCHEMA).create()).doesNotThrowAnyException();
+
+ // Being contained by an ancestor is fine; sitting at exactly an ancestor's location is not.
+ TableIdentifier tableAtNamespaceLocation =
+ TableIdentifier.of(grandchild, "table-at-namespace-location");
+ String grandchildLocation = STORAGE_LOCATION + "/overlap-ancestor-parent/child/grandchild";
+ assertThatThrownBy(
Review Comment:
Should be resolved in [this commit](https://github.com/apache/polaris/pull/5520/changes/76fd63f4d480edd6f587ee074c7320a51507e171).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4021523892
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
// Check for parent (prefix) overlaps. These have shorter keys and are missed by
Review Comment:
This part apparently comes from #4873.
@vigneshio : Could you recap why this logic was needed?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4085367424
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -853,24 +856,52 @@ 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(
Review Comment:
SPI changes are ok per our [Evolution Policy](https://polaris.apache.org/releases/1.7.0/evolution/).
In this case the functionality is extended. Implementations (downstream) are free to ignore the new parameter.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4077701058
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -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;
+ }
+ 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());
Review Comment:
Sure. I did not mean that this code should not be touched. I was looking for more test coverage :sweat_smile: but we can do that in a separate PR, of course.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4077839220
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -630,12 +631,26 @@ <T extends PolarisEntity & LocationBasedEntity> Optional<String> hasOverlappingS
return Optional.empty();
}
+ var catalogId = entity.getCatalogId();
var checkLocation = StorageLocation.of(baseLocation).withoutScheme();
- return hasOverlappingSiblings(entity.getCatalogId(), checkLocation);
+ // The entity's own parent namespaces contain its location by construction; they are not
Review Comment:
`contain` -> `may contain`?
Per email discussion, the check this method performs is mostly relevant in situations when parents do _not_ contain child locations... or at least may not contain. WDYT?
https://lists.apache.org/thread/mypvpsh8l39zo265okhr21rmr2os9oq1
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -853,24 +856,52 @@ 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(
Review Comment:
Ideally we should do this lookup and the overlap table select on the same JDBC Connection. That will give us better consistency. WDYT?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -688,6 +688,18 @@ private void createNamespaceInternal(
.setCreateTimestamp(System.currentTimeMillis())
.setBaseLocation(baseLocation)
.build();
+ EntityResult existingNamespace =
+ getMetaStoreManager()
+ .readEntityByName(
+ getCurrentPolarisContext(),
+ PolarisEntity.toCoreList(resolvedParent.getRawFullPath()),
+ PolarisEntityType.NAMESPACE,
+ PolarisEntitySubType.ANY_SUBTYPE,
+ entity.getName());
+ if (existingNamespace.isSuccess()) {
+ throw new AlreadyExistsException(
Review Comment:
Is this related to the overlap check improvement?
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -697,8 +712,12 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
var prefixKey = prefix.toIndexKey();
var entry = locationsIndex.get(prefixKey);
if (entry != null) {
+ // An ancestor at a strict prefix contains the entity by construction and is not
Review Comment:
similarly, this false positive hit is possible only when "structured locations" are enforced (which may not be the case). Could you update the comment to be more accurate?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -853,24 +856,52 @@ 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(
Review Comment:
However, the only non-test call path leading here is via `validateNoLocationOverlap()`, which has a `List<PolarisEntity> parentPath`... Could we reuse that list for parent IDs and avoid extra selects?
https://github.com/apache/polaris/blob/35e0530e72f9c225c9b1c2ed735f3d2be551ead1/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java#L1540
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4106277503
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
// Check for parent (prefix) overlaps. These have shorter keys and are missed by
Review Comment:
Trying to clean up the state of the comments here, assuming we're good to close this as "resolved" with the context + the thread?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5520:
URL: https://github.com/apache/polaris/pull/5520#issuecomment-5699091647
@andybradshaw : no rush... TBH, I think we're into some substantial cleanup here :sweat_smile:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4027038762
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
// Check for parent (prefix) overlaps. These have shorter keys and are missed by
Review Comment:
Thanks for the analysis, @vigneshio !
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4021550916
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
// Check for parent (prefix) overlaps. These have shorter keys and are missed by
Review Comment:
It looks like the behaviour of `hasOverlappingSiblings()` has evolved to include any location overlap with any entity... hence #5521 :thinking:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4027385574
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
// Check for parent (prefix) overlaps. These have shorter keys and are missed by
Review Comment:
So going to the caller code - `validateNoLocationOverlap()` - if `OPTIMIZED_SIBLING_CHECK` is off, that method only checks true siblings within the same namespace.
https://github.com/apache/polaris/blob/3bdf7dab61e6f72c85b6b77dbed9e5afc9f05d36/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java#L1527
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4030890781
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java:
##########
@@ -295,4 +297,48 @@ public void testParentChildLocationOverlapWithOptimizedSiblingCheck() {
.hasMessageContaining("Unable to create entity at location")
.hasMessageContaining("conflicts with existing table or namespace");
}
+
+ @Test
+ public void testDefaultLocationsUnderAncestorsAreNotOverlaps() {
+ // A namespace's default location is nested under its parent's location, and a table's default
+ // location is nested under its namespace's location. Those ancestors are not siblings and must
+ // never be reported as overlaps, regardless of which sibling-check implementation is active.
+ Namespace parent = Namespace.of("overlap-ancestor-parent");
+ catalog().createNamespace(parent);
+
+ Namespace child = Namespace.of("overlap-ancestor-parent", "child");
+ assertThatCode(() -> catalog().createNamespace(child)).doesNotThrowAnyException();
+
+ Namespace grandchild = Namespace.of("overlap-ancestor-parent", "child", "grandchild");
+ assertThatCode(() -> catalog().createNamespace(grandchild)).doesNotThrowAnyException();
+
+ TableIdentifier table = TableIdentifier.of(grandchild, "table-at-default-location");
+ assertThatCode(() -> catalog().buildTable(table, SCHEMA).create()).doesNotThrowAnyException();
+
+ // Being contained by an ancestor is fine; sitting at exactly an ancestor's location is not.
+ TableIdentifier tableAtNamespaceLocation =
+ TableIdentifier.of(grandchild, "table-at-namespace-location");
+ String grandchildLocation = STORAGE_LOCATION + "/overlap-ancestor-parent/child/grandchild";
+ assertThatThrownBy(
Review Comment:
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.
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -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;
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4085308668
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -688,6 +688,18 @@ private void createNamespaceInternal(
.setCreateTimestamp(System.currentTimeMillis())
.setBaseLocation(baseLocation)
.build();
+ EntityResult existingNamespace =
+ getMetaStoreManager()
+ .readEntityByName(
+ getCurrentPolarisContext(),
+ PolarisEntity.toCoreList(resolvedParent.getRawFullPath()),
+ PolarisEntityType.NAMESPACE,
+ PolarisEntitySubType.ANY_SUBTYPE,
+ entity.getName());
+ if (existingNamespace.isSuccess()) {
+ throw new AlreadyExistsException(
Review Comment:
Not strictly, but it is an API consistency fix as we probably want to report attempting to create the same namespace twice as a `409` instead of a `403`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5520:
URL: https://github.com/apache/polaris/pull/5520#issuecomment-5912780366
@andybradshaw : sorry about the delay... Please fix merge conflicts and note #5654 :sweat_smile:
I'll merge as soon as CI is green.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5520:
URL: https://github.com/apache/polaris/pull/5520#issuecomment-5833154421
@andybradshaw : #5596 has been merged. Please rebase.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4021432096
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -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;
+ }
+ 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());
Review Comment:
The old code for this case does not break the new test : `testDefaultLocationsUnderAncestorsAreNotOverlaps` ... Why do we have to alter it?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4021367671
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -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;
Review Comment:
Do we have a test for this case? (I might have missed it ๐
)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4027549317
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java:
##########
@@ -692,29 +728,11 @@ Optional<String> hasOverlappingSiblings(long catalogId, String checkLocation) {
// Check for parent (prefix) overlaps. These have shorter keys and are missed by
Review Comment:
Cf. https://lists.apache.org/thread/m0jy5mh7wch0wxm5gl2q52g9g4s6krr8
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "andybradshaw (via GitHub)" <gi...@apache.org>.
andybradshaw commented on PR #5520:
URL: https://github.com/apache/polaris/pull/5520#issuecomment-5766653462
Apologies for the delay. I believe I've addressed all of the feedback from the last round of reviews. Let me know if there are other things you'd like to discuss! ๐
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4085379690
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -688,6 +688,18 @@ private void createNamespaceInternal(
.setCreateTimestamp(System.currentTimeMillis())
.setBaseLocation(baseLocation)
.build();
+ EntityResult existingNamespace =
+ getMetaStoreManager()
+ .readEntityByName(
+ getCurrentPolarisContext(),
+ PolarisEntity.toCoreList(resolvedParent.getRawFullPath()),
+ PolarisEntityType.NAMESPACE,
+ PolarisEntitySubType.ANY_SUBTYPE,
+ entity.getName());
+ if (existingNamespace.isSuccess()) {
+ throw new AlreadyExistsException(
Review Comment:
Good point, but would you mind moving it to a separate PR?.. just for clarity.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OPTIMIZED_SIBLING_CHECK ancestor overlap [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5520:
URL: https://github.com/apache/polaris/pull/5520#discussion_r4085422893
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java:
##########
@@ -397,18 +397,33 @@ boolean hasChildren(
long parentId);
/**
- * Check if the specified IcebergTableLikeEntity / NamespaceEntity has any sibling entities which
- * share a base location
+ * Check whether the base location of the specified IcebergTableLikeEntity / NamespaceEntity
+ * overlaps the base location of another entity in the same catalog.
+ *
+ * <p>Despite the name, implementations do not limit the search to siblings under the same parent.
+ * They consider every table, view, and namespace in the catalog whose base location contains,
+ * equals, or is contained by the entity's base location. This is the check behind {@code
+ * OPTIMIZED_SIBLING_CHECK}; the fallback used when that flag is off inspects only the entities
+ * directly under the same parent.
+ *
+ * <p>The entity's own ancestors, given by {@code catalogPath}, are excluded: a parent namespace
+ * whose location strictly contains the entity's location is not reported, since default locations
+ * nest under the parent's location. An ancestor whose location equals the entity's location is
+ * still reported.
*
* @param callContext the polaris call context
- * @param entity the entity to check for overlapping siblings for
- * @return Optional.of(Optional.of(location)) if the parent entity has children,
- * Optional.of(Optional.empty()) if not, and Optional.empty() if the metastore doesn't support
- * this operation
+ * @param catalogPath the resolved path of the entity's ancestors: the catalog first, then each
+ * parent namespace
+ * @param entity the entity whose base location to check
+ * @return Optional.of(Optional.of(location)) with the base location of a conflicting entity,
+ * Optional.of(Optional.empty()) if there is no conflict, and Optional.empty() if the
+ * metastore doesn't support this operation
*/
default <T extends PolarisEntity & LocationBasedEntity>
Optional<Optional<String>> hasOverlappingSiblings(
- @NonNull PolarisCallContext callContext, T entity) {
+ @NonNull PolarisCallContext callContext,
+ @NonNull List<PolarisEntityCore> catalogPath,
Review Comment:
nit: the javadoc on `catalogPath` is clear, but the parameter name itself may be confused to mean that it's the full path _including_ the entity. How about `parentPath`?
Note that javadoc is rarely propagated to implementation classes.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org