You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@polaris.apache.org by ad...@apache.org on 2026/10/26 13:29:08 UTC
(polaris) branch main updated: Ignore the entity's own ID in the optimized sibling check (#5858)
This is an automated email from the ASF dual-hosted git repository.
adutra pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new 421062c85b Ignore the entity's own ID in the optimized sibling check (#5858)
421062c85b is described below
commit 421062c85b328424d8b32ca10212333a7fa8b4dc
Author: Alexandre Dutra <ad...@apache.org>
AuthorDate: Mon Oct 26 14:29:03 2026 +0100
Ignore the entity's own ID in the optimized sibling check (#5858)
Change hasOverlappingSiblings so that the entity's own ID is ignored; this prevents operations such as renames from conflicting with the entity's own persisted location.
---
.../nosql/metastore/NoSqlMetaStore.java | 6 +-
.../nosql/metastore/TestNoSqlMetaStoreManager.java | 23 +++++++
.../relational/jdbc/JdbcBasePersistenceImpl.java | 3 +
...toreManagerWithJdbcBasePersistenceImplTest.java | 11 ++++
.../polaris/core/persistence/BasePersistence.java | 4 ++
.../core/persistence/PolarisMetaStoreManager.java | 3 +-
.../TreeMapTransactionalPersistenceImpl.java | 3 +
.../PolarisTreeMapMetaStoreManagerTest.java | 70 ++++++++++++++++++++++
.../catalog/iceberg/LocalIcebergCatalog.java | 15 +++--
.../AbstractLocalIcebergCatalogOverlapTest.java | 57 ++++++++++++++++++
10 files changed, 188 insertions(+), 7 deletions(-)
diff --git a/persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java b/persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java
index 2d73c4f6ae..6c61c021da 100644
--- a/persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java
+++ b/persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStore.java
@@ -643,11 +643,11 @@ class NoSqlMetaStore extends NonFunctionalBasePersistence {
// follow the namespace tree, as default locations do. Such an ancestor is not a sibling.
var ancestorIds = parentPath.stream().map(PolarisEntityCore::getId).collect(Collectors.toSet());
- return hasOverlappingSiblings(catalogId, checkLocation, ancestorIds);
+ return hasOverlappingSiblings(catalogId, checkLocation, ancestorIds, entity.getId());
}
Optional<String> hasOverlappingSiblings(
- long catalogId, String checkLocation, Set<Long> ancestorIds) {
+ long catalogId, String checkLocation, Set<Long> ancestorIds, long entityId) {
return memoizedIndexedAccess
.catalogContent(catalogId)
.refObj()
@@ -680,6 +680,7 @@ class NoSqlMetaStore extends NonFunctionalBasePersistence {
var conflicting =
elem.value().entityIds().stream()
+ .filter(id -> id != entityId)
.map(IndexKey::key)
.map(byId::get)
.filter(Objects::nonNull)
@@ -715,6 +716,7 @@ class NoSqlMetaStore extends NonFunctionalBasePersistence {
var strictPrefix = i < locationIdentifier.length();
var conflicting =
entry.entityIds().stream()
+ .filter(id -> id != entityId)
.filter(id -> !strictPrefix || !ancestorIds.contains(id))
.map(IndexKey::key)
.map(byId::get)
diff --git a/persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java b/persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java
index a64287634d..1016fd48e8 100644
--- a/persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java
+++ b/persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java
@@ -271,6 +271,29 @@ public class TestNoSqlMetaStoreManager extends BasePolarisMetaStoreManagerTest {
.isPresent()
.contains(Optional.of("s3://bucket/foo/"));
+ // The same location is a conflict, unless the entity is the persisted one (e.g. a rename).
+ var renamedFoo =
+ new NamespaceEntity.Builder(Namespace.of("renamed"))
+ .setCatalogId(catalog.getId())
+ .setBaseLocation("s3://bucket/foo/")
+ .build();
+ soft.assertThat(
+ metaStore.hasOverlappingSiblings(
+ callContext, List.of(PolarisEntity.toCore(catalog)), renamedFoo))
+ .isPresent()
+ .contains(Optional.of("s3://bucket/foo/"));
+ var persistedFoo =
+ new NamespaceEntity.Builder(Namespace.of("renamed"))
+ .setId(nsFoo.getEntity().getId())
+ .setCatalogId(catalog.getId())
+ .setBaseLocation("s3://bucket/foo/")
+ .build();
+ soft.assertThat(
+ metaStore.hasOverlappingSiblings(
+ callContext, List.of(PolarisEntity.toCore(catalog)), persistedFoo))
+ .isPresent()
+ .contains(Optional.empty());
+
var nsBar =
createEntity(
List.of(catalog),
diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
index e8c5e4e6c3..dc1da5b005 100644
--- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
+++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
@@ -819,6 +819,9 @@ public class JdbcBasePersistenceImpl implements BasePersistence, IntegrationPers
StorageLocation entityLocation = StorageLocation.of(entity.getBaseLocation());
for (PolarisBaseEntity result : results) {
+ if (result.getId() == entity.getId()) {
+ continue;
+ }
// JDBC materializes persisted rows as PolarisBaseEntity. Resolve the sibling location
// via PolarisEntityUtils instead of casting to LocationBasedEntity.
Optional<StorageLocation> resultLocation =
diff --git a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/AtomicMetastoreManagerWithJdbcBasePersistenceImplTest.java b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/AtomicMetastoreManagerWithJdbcBasePersistenceImplTest.java
index fd26982ff2..ae4a0aac28 100644
--- a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/AtomicMetastoreManagerWithJdbcBasePersistenceImplTest.java
+++ b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/AtomicMetastoreManagerWithJdbcBasePersistenceImplTest.java
@@ -147,6 +147,17 @@ public abstract class AtomicMetastoreManagerWithJdbcBasePersistenceImplTest
metaStoreManager.hasOverlappingSiblings(
callContext, List.of(PolarisEntity.toCore(catalog)), nonOverlappingNamespace))
.contains(Optional.empty());
+
+ // The persisted entity itself, e.g. when renamed, does not conflict with its own location.
+ TestLocationBasedEntity renamedNamespace =
+ new TestLocationBasedEntity(
+ buildLocationBasedNamespace(
+ catalog, existingNamespace.getId(), "renamed", "s3://bucket/warehouse/existing/"));
+
+ Assertions.assertThat(
+ metaStoreManager.hasOverlappingSiblings(
+ callContext, List.of(PolarisEntity.toCore(catalog)), renamedNamespace))
+ .contains(Optional.empty());
}
@Test
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
index 5aaa1f3f0d..75e89a548a 100644
--- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
+++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
@@ -411,6 +411,10 @@ public interface BasePersistence extends PolicyMappingPersistence {
* nest under the parent's location. An ancestor whose location equals the entity's location is
* still reported.
*
+ * <p>The entity itself, identified by its id, is never reported. Implementations should avoid
+ * checking the entity against itself when it is already persisted, e.g. on a location update or a
+ * rename, so that it does not conflict with its own persisted location.
+ *
* @param callContext the polaris call context
* @param parentPath the entity's resolved parent path: the catalog first, then each parent
* namespace, not including the entity itself
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java
index d48786457e..f3ca7b3f16 100644
--- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java
+++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java
@@ -468,7 +468,8 @@ public interface PolarisMetaStoreManager
* Check whether the base location of the specified IcebergTableLikeEntity / NamespaceEntity
* overlaps the base location of another table, view, or namespace in the same catalog. The check
* is not limited to entities under the same parent; see {@link
- * BasePersistence#hasOverlappingSiblings} for the exact semantics.
+ * BasePersistence#hasOverlappingSiblings} for the exact semantics. Implementations should avoid
+ * checking the entity against itself in case of updates or renames.
*
* @param callContext the polaris call context
* @param parentPath the entity's resolved parent path: the catalog first, then each parent
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java
index e6a56c695d..abea2b4c1b 100644
--- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java
+++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java
@@ -688,6 +688,9 @@ public class TreeMapTransactionalPersistenceImpl extends AbstractTransactionalPe
parentPath.stream().map(PolarisEntityCore::getId).collect(Collectors.toSet());
for (PolarisBaseEntity siblingEntity : allEntities) {
+ if (siblingEntity.getId() == entity.getId()) {
+ continue;
+ }
Optional<StorageLocation> maybeSiblingLocationWithoutScheme =
getEntityLocationWithoutScheme(siblingEntity).map(StorageLocation::of);
if (maybeSiblingLocationWithoutScheme.isPresent()) {
diff --git a/polaris-core/src/test/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreManagerTest.java b/polaris-core/src/test/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreManagerTest.java
index 4ecfa027f5..6437e1eb32 100644
--- a/polaris-core/src/test/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreManagerTest.java
+++ b/polaris-core/src/test/java/org/apache/polaris/core/persistence/PolarisTreeMapMetaStoreManagerTest.java
@@ -19,13 +19,24 @@
package org.apache.polaris.core.persistence;
import static org.apache.polaris.core.persistence.PrincipalSecretsGenerator.RANDOM_SECRETS;
+import static org.assertj.core.api.Assertions.assertThat;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.PolarisDefaultDiagServiceImpl;
import org.apache.polaris.core.PolarisDiagnostics;
+import org.apache.polaris.core.entity.LocationBasedEntity;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.apache.polaris.core.entity.PolarisEntitySubType;
+import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.persistence.transactional.TransactionalMetaStoreManagerImpl;
import org.apache.polaris.core.persistence.transactional.TreeMapMetaStore;
import org.apache.polaris.core.persistence.transactional.TreeMapTransactionalPersistenceImpl;
+import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
public class PolarisTreeMapMetaStoreManagerTest extends BasePolarisMetaStoreManagerTest {
@@ -41,4 +52,63 @@ public class PolarisTreeMapMetaStoreManagerTest extends BasePolarisMetaStoreMana
PolarisCallContext callCtx = new PolarisCallContext(() -> "testRealm", metaStore);
return new PolarisTestMetaStoreManager(metaStoreManager, callCtx);
}
+
+ @Test
+ void hasOverlappingSiblingsIgnoresEntityItself() {
+ var metaStoreManager = polarisTestMetaStoreManager.polarisMetaStoreManager();
+ var callContext = polarisTestMetaStoreManager.polarisCallContext();
+ PolarisBaseEntity catalog =
+ new PolarisBaseEntity(
+ PolarisEntityConstants.getNullId(),
+ metaStoreManager.generateNewEntityId(callContext).getId(),
+ PolarisEntityType.CATALOG,
+ PolarisEntitySubType.NULL_SUBTYPE,
+ PolarisEntityConstants.getRootEntityId(),
+ "overlap_test_catalog");
+ catalog = metaStoreManager.createCatalog(callContext, catalog, List.of()).getCatalog();
+
+ PolarisBaseEntity existing =
+ buildNamespace(catalog, metaStoreManager.generateNewEntityId(callContext).getId());
+ metaStoreManager.createEntityIfNotExists(
+ callContext, List.of(PolarisEntity.toCore(catalog)), existing);
+
+ var parentPath = List.of(PolarisEntity.toCore(catalog));
+
+ // A different entity at the same location is a conflict.
+ var other = new LocationBasedTestEntity(buildNamespace(catalog, existing.getId() + 1));
+ var conflict = metaStoreManager.hasOverlappingSiblings(callContext, parentPath, other);
+ assertThat(conflict).isPresent();
+ assertThat(conflict.get()).isPresent();
+
+ // The persisted entity itself, e.g. when renamed, is not a conflict.
+ var self = new LocationBasedTestEntity(buildNamespace(catalog, existing.getId()));
+ assertThat(metaStoreManager.hasOverlappingSiblings(callContext, parentPath, self))
+ .contains(Optional.empty());
+ }
+
+ private static PolarisBaseEntity buildNamespace(PolarisBaseEntity catalog, long entityId) {
+ return new PolarisBaseEntity.Builder()
+ .catalogId(catalog.getId())
+ .parentId(catalog.getId())
+ .id(entityId)
+ .typeCode(PolarisEntityType.NAMESPACE.getCode())
+ .subTypeCode(PolarisEntitySubType.NULL_SUBTYPE.getCode())
+ .name("existing")
+ .propertiesAsMap(
+ Map.of(PolarisEntityConstants.ENTITY_BASE_LOCATION, "s3://bucket/warehouse/existing/"))
+ .internalPropertiesAsMap(Map.of())
+ .build();
+ }
+
+ private static class LocationBasedTestEntity extends PolarisEntity
+ implements LocationBasedEntity {
+ LocationBasedTestEntity(PolarisBaseEntity sourceEntity) {
+ super(sourceEntity);
+ }
+
+ @Override
+ public String getBaseLocation() {
+ return getPropertiesAsMap().get(PolarisEntityConstants.ENTITY_BASE_LOCATION);
+ }
+ }
}
diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java
index b7d07fbcd3..45314640bd 100644
--- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java
+++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java
@@ -915,9 +915,9 @@ public class LocalIcebergCatalog extends BaseMetastoreViewCatalog
// Compare normalized (slash-terminated) locations so that merely normalizing a legacy
// slash-less location does not count as a move. Otherwise an unrelated property update on a
// namespace stored before Polaris always appended a slash would flip locationChanged to true
- // and run the overlap check against the namespace's own still-persisted slash-less row (which
- // the optimized sibling check does not exclude), wrongly rejecting the update. A genuine move
- // still differs after normalization and is validated.
+ // and run the overlap check against the namespace's own still-persisted slash-less row,
+ // wrongly rejecting the update. A genuine move still differs after normalization and is
+ // validated.
boolean locationChanged =
!Objects.equal(
StorageLocation.ensureTrailingSlash(NamespaceEntity.of(entity).getBaseLocation()),
@@ -1439,11 +1439,18 @@ public class LocalIcebergCatalog extends BaseMetastoreViewCatalog
LOGGER.debug("Validating no overlap with sibling tables or namespaces");
// Create a fake IcebergTableLikeEntity to check for overlap, since no real entity
- // has been created yet.
+ // has been created yet. If the table or view already exists, the fake entity carries its id,
+ // so that the check does not conflict with the entity's own persisted location. When the
+ // leaf is not a table-like entity, it is the parent namespace or catalog, which must not be
+ // ignored.
var lastNamespace = resolvedNamespace.getLast();
IcebergTableLikeEntity virtualEntity =
IcebergTableLikeEntity.of(
new PolarisEntity.Builder()
+ .setId(
+ entity.getType() == PolarisEntityType.TABLE_LIKE
+ ? entity.getId()
+ : PolarisEntityConstants.getNullId())
.setName(identifier.name())
.setType(PolarisEntityType.TABLE_LIKE)
.setSubType(PolarisEntitySubType.ICEBERG_TABLE)
diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java
index 36ec93f869..0014f8c382 100644
--- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java
+++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java
@@ -34,6 +34,7 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.Table;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.AlreadyExistsException;
@@ -45,6 +46,7 @@ import org.apache.polaris.core.admin.model.AwsStorageConfigInfo;
import org.apache.polaris.core.admin.model.CreateCatalogRequest;
import org.apache.polaris.core.admin.model.StorageConfigInfo;
import org.apache.polaris.core.auth.PolarisPrincipal;
+import org.apache.polaris.core.config.BehaviorChangeConfiguration;
import org.apache.polaris.core.config.FeatureConfiguration;
import org.apache.polaris.core.config.RealmConfig;
import org.apache.polaris.core.context.CallContext;
@@ -174,6 +176,9 @@ public abstract class AbstractLocalIcebergCatalogOverlapTest {
"true")
.addProperty(
FeatureConfiguration.DROP_WITH_PURGE_ENABLED.catalogConfig(), "true")
+ .addProperty(
+ BehaviorChangeConfiguration.ALLOW_NAMESPACE_CUSTOM_LOCATION.catalogConfig(),
+ "true")
.setStorageConfigurationInfo(realmConfig, storageConfigModel)
.build()
.asCatalog(serviceIdentityProvider)));
@@ -412,4 +417,56 @@ public abstract class AbstractLocalIcebergCatalogOverlapTest {
.isInstanceOf(ForbiddenException.class)
.hasMessageContaining("conflicts with existing table or namespace");
}
+
+ @Test
+ public void testMovingNamespaceUnderItsOldLocationIsNotOverlap() {
+ // A namespace whose location is updated must not conflict with its own persisted location, so
+ // moving it to a subpath of its old location is allowed.
+ Namespace ns = Namespace.of("overlap-move-namespace");
+ String oldLocation = STORAGE_LOCATION + "/overlap-move-namespace-old";
+ catalog().createNamespace(ns, Map.of(PolarisEntityConstants.ENTITY_BASE_LOCATION, oldLocation));
+
+ String newLocation = oldLocation + "/moved";
+ assertThatCode(
+ () ->
+ catalog()
+ .setProperties(
+ ns, Map.of(PolarisEntityConstants.ENTITY_BASE_LOCATION, newLocation)))
+ .doesNotThrowAnyException();
+
+ // Other namespaces still conflict with the new location.
+ Namespace conflicting = Namespace.of("overlap-move-namespace-conflicting");
+ assertThatThrownBy(
+ () ->
+ catalog()
+ .createNamespace(
+ conflicting,
+ Map.of(PolarisEntityConstants.ENTITY_BASE_LOCATION, newLocation)))
+ .isInstanceOf(ForbiddenException.class)
+ .hasMessageContaining("conflicts with existing table or namespace");
+ }
+
+ @Test
+ public void testMovingTableUnderItsOldLocationIsNotOverlap() {
+ // Same as above for a table whose location is updated by a commit.
+ Namespace ns = Namespace.of("overlap-move-table-ns");
+ catalog().createNamespace(ns);
+ TableIdentifier identifier = TableIdentifier.of(ns, "t");
+ String oldLocation = STORAGE_LOCATION + "/overlap-move-table-old";
+ Table table = catalog().buildTable(identifier, SCHEMA).withLocation(oldLocation).create();
+
+ String newLocation = oldLocation + "/moved";
+ assertThatCode(() -> table.updateLocation().setLocation(newLocation).commit())
+ .doesNotThrowAnyException();
+
+ // Other tables still conflict with the new location.
+ assertThatThrownBy(
+ () ->
+ catalog()
+ .buildTable(TableIdentifier.of(ns, "conflicting"), SCHEMA)
+ .withLocation(newLocation)
+ .create())
+ .isInstanceOf(ForbiddenException.class)
+ .hasMessageContaining("conflicts with existing table or namespace");
+ }
}