From 220bfddbf4f7ee536721534ed4024a82587889c1 Mon Sep 17 00:00:00 2001 From: Christopher Lambert <1204398+XN137@users.noreply.github.com> Date: Sat, 9 Aug 2025 08:32:56 +0200 Subject: [PATCH 1/4] Add PolarisMetaStoreManager.loadEntities currently `PolarisMetaStoreManager.listEntities` only exposes a limited subset of the underlying `BasePersistence.listEntities` functionality. most of the callers have to post-process the `EntityNameLookupRecord` of `ListEntitiesResult` and call `PolarisMetaStoreManager.loadEntity` on the individual items sequentually to transform and filter them. this is bad for the following reasons: - suboptimal performance as we run N+1 queries to basically load every entity twice from the persistence backend - suffering from race-conditions when entities get dropped between the `listEntities` and `loadEntity` call - a lot of repeated code in all the callers (of which only some are dealing with the race-condition by filtering out null values) as a solution we add `PolarisMetaStoreManager.loadEntities` that takes advantage of the already existing `BasePersistence` methods. we rename one of the `listEntities` methods to `loadEntities` for consistency. since many callers dont need paging and want the result as a list, we add `PolarisMetaStoreManager.loadEntitiesAll` as a convenient wrapper. we also remove the `PolarisEntity.nameAndId` method as callers who only need name and id should not be loading the full entity to begin with. note we rework `testCatalogNotReturnedWhenDeletedAfterListBeforeGet` from `ManagementServiceTest` because the simulated race-condition scenario can no longer happen. --- ...olarisEclipseLinkMetaStoreSessionImpl.java | 2 +- .../jdbc/JdbcBasePersistenceImpl.java | 4 +- .../polaris/core/entity/PolarisEntity.java | 5 -- .../AtomicOperationMetaStoreManager.java | 44 +++++++++++- .../core/persistence/BasePersistence.java | 4 +- .../persistence/PolarisMetaStoreManager.java | 56 +++++++++++++++ .../TransactionWorkspaceMetaStoreManager.java | 18 ++++- .../AbstractTransactionalPersistence.java | 4 +- .../TransactionalMetaStoreManagerImpl.java | 67 +++++++++++++++++- .../TransactionalPersistence.java | 4 +- .../TreeMapTransactionalPersistenceImpl.java | 12 +--- .../BasePolarisMetaStoreManagerTest.java | 34 +++------ .../service/admin/PolarisAdminService.java | 70 +++++++------------ .../catalog/iceberg/IcebergCatalog.java | 10 ++- .../service/catalog/policy/PolicyCatalog.java | 38 +++------- .../service/admin/ManagementServiceTest.java | 27 +++---- 16 files changed, 244 insertions(+), 155 deletions(-) diff --git a/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java b/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java index 9929c2da3e..75c4c4ad4f 100644 --- a/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java +++ b/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java @@ -428,7 +428,7 @@ public List lookupEntityActiveBatchInCurrentTxn( } @Override - public @Nonnull Page listEntitiesInCurrentTxn( + public @Nonnull Page loadEntitiesInCurrentTxn( @Nonnull PolarisCallContext callCtx, long catalogId, long parentId, 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 1babeb03ee..71f1d2e7aa 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 @@ -431,7 +431,7 @@ public Page listEntities( @Nonnull PolarisEntitySubType entitySubType, @Nonnull PageToken pageToken) { // TODO: only fetch the properties required for creating an EntityNameLookupRecord - return listEntities( + return loadEntities( callCtx, catalogId, parentId, @@ -444,7 +444,7 @@ public Page listEntities( @Nonnull @Override - public Page listEntities( + public Page loadEntities( @Nonnull PolarisCallContext callCtx, long catalogId, long parentId, diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java index 1d5def481a..5dccbe1bf7 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java @@ -223,11 +223,6 @@ public PolarisEntitySubType getSubType() { return PolarisEntitySubType.fromCode(getSubTypeCode()); } - @JsonIgnore - public NameAndId nameAndId() { - return new NameAndId(name, id); - } - @Override public String toString() { return "name=" diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java index 8af4c654dd..c31efd6692 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java @@ -31,6 +31,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Collectors; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.AsyncTaskType; @@ -717,10 +718,47 @@ private void revokeGrantRecord( // with sensitive data; but the window of inconsistency is only the duration of a single // in-flight request (the cache-based resolution follows a different path entirely). - // done return ListEntitiesResult.fromPage(resultPage); } + /** {@inheritDoc} */ + @Override + public @Nonnull Page loadEntities( + @Nonnull PolarisCallContext callCtx, + @Nullable List catalogPath, + @Nonnull PolarisEntityType entityType, + @Nonnull PolarisEntitySubType entitySubType, + @Nonnull Predicate entityFilter, + @Nonnull Function transformer, + @Nonnull PageToken pageToken) { + // get meta store we should be using + BasePersistence ms = callCtx.getMetaStore(); + + // return list of active entities + // TODO: Clean up shared logic for catalogId/parentId + long catalogId = catalogPath == null || catalogPath.isEmpty() ? 0L : catalogPath.get(0).getId(); + long parentId = + catalogPath == null || catalogPath.isEmpty() + ? 0L + : catalogPath.get(catalogPath.size() - 1).getId(); + + // TODO: Use post-validation to enforce consistent view against catalogPath. In the + // meantime, happens-before ordering semantics aren't guaranteed during high-concurrency + // race conditions, such as first revoking a grant on a namespace before adding a table + // with sensitive data; but the window of inconsistency is only the duration of a single + // in-flight request (the cache-based resolution follows a different path entirely). + + return ms.loadEntities( + callCtx, + catalogId, + parentId, + entityType, + entitySubType, + entityFilter, + transformer, + pageToken); + } + /** {@inheritDoc} */ @Override public @Nonnull CreatePrincipalResult createPrincipal( @@ -1166,7 +1204,7 @@ private void revokeGrantRecord( // get the list of catalog roles, at most 2 List catalogRoles = - ms.listEntities( + ms.loadEntities( callCtx, catalogId, catalogId, @@ -1488,7 +1526,7 @@ private void revokeGrantRecord( // find all available tasks Page availableTasks = - ms.listEntities( + ms.loadEntities( callCtx, PolarisEntityConstants.getRootEntityId(), PolarisEntityConstants.getRootEntityId(), 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 1e8d2abf81..76ceb0520d 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 @@ -290,7 +290,7 @@ Page listEntities( @Nonnull PageToken pageToken); /** - * List entities where some predicate returns true and transform the entities with a function + * Load entities where some predicate returns true and transform the entities with a function * * @param callCtx call context * @param catalogId catalog id for that entity, NULL_ID if the entity is top-level @@ -304,7 +304,7 @@ Page listEntities( * @return the list of entities for which the predicate returns true */ @Nonnull - Page listEntities( + Page loadEntities( @Nonnull PolarisCallContext callCtx, long catalogId, long parentId, 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 67175e21fb..206d3e3ae4 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 @@ -23,6 +23,8 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.function.Function; +import java.util.function.Predicate; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.auth.PolarisGrantManager; import org.apache.polaris.core.auth.PolarisSecretsManager; @@ -47,6 +49,7 @@ import org.apache.polaris.core.persistence.dao.entity.GenerateEntityIdResult; import org.apache.polaris.core.persistence.dao.entity.ListEntitiesResult; import org.apache.polaris.core.persistence.dao.entity.ResolvedEntityResult; +import org.apache.polaris.core.persistence.pagination.Page; import org.apache.polaris.core.persistence.pagination.PageToken; import org.apache.polaris.core.policy.PolarisPolicyMappingManager; import org.apache.polaris.core.storage.PolarisCredentialVendor; @@ -129,6 +132,59 @@ ListEntitiesResult listEntities( @Nonnull PolarisEntitySubType entitySubType, @Nonnull PageToken pageToken); + /** + * Load entities where some predicate returns true and transform the entities with a function + * + * @param callCtx call context + * @param catalogPath path inside a catalog. If null or empty, the entities to list are top-level, + * like catalogs + * @param entityType type of entities to list + * @param entitySubType subType of entities to list (or ANY_SUBTYPE) + * @param entityFilter the filter to be applied to each entity. Only entities where the predicate + * returns true are returned in the list + * @param transformer the transformation function applied to the {@link PolarisBaseEntity} before + * returning + * @return the paged list of entities for which the predicate returns true + */ + @Nonnull + Page loadEntities( + @Nonnull PolarisCallContext callCtx, + @Nullable List catalogPath, + @Nonnull PolarisEntityType entityType, + @Nonnull PolarisEntitySubType entitySubType, + @Nonnull Predicate entityFilter, + @Nonnull Function transformer, + @Nonnull PageToken pageToken); + + /** + * Load all entities and transform the entities with a function into an unpaged list + * + * @param callCtx call context + * @param catalogPath path inside a catalog. If null or empty, the entities to list are top-level, + * like catalogs + * @param entityType type of entities to list + * @param entitySubType subType of entities to list (or ANY_SUBTYPE) + * @param transformer the transformation function applied to the {@link PolarisBaseEntity} before + * returning + * @return the full list of entities + */ + default @Nonnull List loadEntitiesAll( + @Nonnull PolarisCallContext callCtx, + @Nullable List catalogPath, + @Nonnull PolarisEntityType entityType, + @Nonnull PolarisEntitySubType entitySubType, + @Nonnull Function transformer) { + return loadEntities( + callCtx, + catalogPath, + entityType, + entitySubType, + e -> true, + transformer, + PageToken.readEverything()) + .items(); + } + /** * Generate a new unique id that can be used by the Polaris client when it needs to create a new * entity diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java index f707d1c29f..eee3a34b27 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java @@ -26,6 +26,8 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.function.Function; +import java.util.function.Predicate; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; import org.apache.polaris.core.entity.LocationBasedEntity; @@ -53,6 +55,7 @@ import org.apache.polaris.core.persistence.dao.entity.PrivilegeResult; import org.apache.polaris.core.persistence.dao.entity.ResolvedEntityResult; import org.apache.polaris.core.persistence.dao.entity.ScopedCredentialsResult; +import org.apache.polaris.core.persistence.pagination.Page; import org.apache.polaris.core.persistence.pagination.PageToken; import org.apache.polaris.core.policy.PolicyEntity; import org.apache.polaris.core.policy.PolicyType; @@ -119,7 +122,7 @@ public EntityResult readEntityByName( } @Override - public ListEntitiesResult listEntities( + public @Nonnull ListEntitiesResult listEntities( @Nonnull PolarisCallContext callCtx, @Nullable List catalogPath, @Nonnull PolarisEntityType entityType, @@ -129,6 +132,19 @@ public ListEntitiesResult listEntities( return null; } + @Override + public @Nonnull Page loadEntities( + @Nonnull PolarisCallContext callCtx, + @Nullable List catalogPath, + @Nonnull PolarisEntityType entityType, + @Nonnull PolarisEntitySubType entitySubType, + @Nonnull Predicate entityFilter, + @Nonnull Function transformer, + @Nonnull PageToken pageToken) { + callCtx.getDiagServices().fail("illegal_method_in_transaction_workspace", "loadEntities"); + return null; + } + @Override public GenerateEntityIdResult generateNewEntityId(@Nonnull PolarisCallContext callCtx) { diagnostics.fail("illegal_method_in_transaction_workspace", "generateNewEntityId"); diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/AbstractTransactionalPersistence.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/AbstractTransactionalPersistence.java index 138b3f4a75..d5b323fabe 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/AbstractTransactionalPersistence.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/AbstractTransactionalPersistence.java @@ -383,7 +383,7 @@ public Page listEntities( /** {@inheritDoc} */ @Override @Nonnull - public Page listEntities( + public Page loadEntities( @Nonnull PolarisCallContext callCtx, long catalogId, long parentId, @@ -395,7 +395,7 @@ public Page listEntities( return runInReadTransaction( callCtx, () -> - this.listEntitiesInCurrentTxn( + this.loadEntitiesInCurrentTxn( callCtx, catalogId, parentId, diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java index ee202d3ca7..b40aed0838 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java @@ -30,6 +30,7 @@ import java.util.Optional; import java.util.Set; import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Collectors; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; @@ -699,7 +700,6 @@ private void bootstrapPolarisService( entitySubType, pageToken); - // done return ListEntitiesResult.fromPage(resultPage); } @@ -720,6 +720,67 @@ private void bootstrapPolarisService( () -> listEntities(callCtx, ms, catalogPath, entityType, entitySubType, pageToken)); } + /** + * See {@link PolarisMetaStoreManager#loadEntities(PolarisCallContext, List, PolarisEntityType, + * PolarisEntitySubType, Predicate, Function, PageToken)} + */ + private @Nonnull Page loadEntities( + @Nonnull PolarisCallContext callCtx, + @Nonnull TransactionalPersistence ms, + @Nullable List catalogPath, + @Nonnull PolarisEntityType entityType, + @Nonnull PolarisEntitySubType entitySubType, + @Nonnull Predicate entityFilter, + @Nonnull Function transformer, + @Nonnull PageToken pageToken) { + // first resolve again the catalogPath to that entity + PolarisEntityResolver resolver = new PolarisEntityResolver(callCtx, ms, catalogPath); + + // throw if we failed to resolve + if (resolver.isFailure()) { + throw new IllegalArgumentException("Failed to resolve catalogPath: " + catalogPath); + } + + // return list of active entities + return ms.loadEntitiesInCurrentTxn( + callCtx, + resolver.getCatalogIdOrNull(), + resolver.getParentId(), + entityType, + entitySubType, + entityFilter, + transformer, + pageToken); + } + + /** {@inheritDoc} */ + @Override + public @Nonnull Page loadEntities( + @Nonnull PolarisCallContext callCtx, + @Nullable List catalogPath, + @Nonnull PolarisEntityType entityType, + @Nonnull PolarisEntitySubType entitySubType, + @Nonnull Predicate entityFilter, + @Nonnull Function transformer, + @Nonnull PageToken pageToken) { + // get meta store we should be using + TransactionalPersistence ms = ((TransactionalPersistence) callCtx.getMetaStore()); + + // run operation in a read transaction + return ms.runInReadTransaction( + callCtx, + () -> + loadEntities( + callCtx, + ms, + catalogPath, + entityType, + entitySubType, + entityFilter, + transformer, + pageToken)); + } + /** {@link #createPrincipal(PolarisCallContext, PolarisBaseEntity)} */ private @Nonnull CreatePrincipalResult createPrincipal( @Nonnull PolarisCallContext callCtx, @@ -1335,7 +1396,7 @@ private void bootstrapPolarisService( // get the list of catalog roles, at most 2 List catalogRoles = - ms.listEntitiesInCurrentTxn( + ms.loadEntitiesInCurrentTxn( callCtx, catalogId, catalogId, @@ -1901,7 +1962,7 @@ private PolarisEntityResolver resolveSecurableToRoleGrant( // find all available tasks Page availableTasks = - ms.listEntitiesInCurrentTxn( + ms.loadEntitiesInCurrentTxn( callCtx, PolarisEntityConstants.getRootEntityId(), PolarisEntityConstants.getRootEntityId(), diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java index 6eacd62db4..eb3371c66c 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java @@ -214,7 +214,7 @@ List lookupEntityVersionsInCurrentTxn( @Nonnull PolarisEntityType entityType, @Nonnull PolarisEntitySubType entitySubType, @Nonnull PageToken pageToken) { - return listEntitiesInCurrentTxn( + return loadEntitiesInCurrentTxn( callCtx, catalogId, parentId, @@ -227,7 +227,7 @@ List lookupEntityVersionsInCurrentTxn( /** See {@link org.apache.polaris.core.persistence.BasePersistence#listEntities} */ @Nonnull - Page listEntitiesInCurrentTxn( + Page loadEntitiesInCurrentTxn( @Nonnull PolarisCallContext callCtx, long catalogId, long parentId, 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 7fdd94b9a7..651800621a 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 @@ -288,15 +288,7 @@ public EntityNameLookupRecord lookupEntityActiveInCurrentTxn( entityActiveKey.getName())); // return record - return (entity == null) - ? null - : new EntityNameLookupRecord( - entity.getCatalogId(), - entity.getId(), - entity.getParentId(), - entity.getName(), - entity.getTypeCode(), - entity.getSubTypeCode()); + return entity == null ? null : new EntityNameLookupRecord(entity); } /** {@inheritDoc} */ @@ -312,7 +304,7 @@ public List lookupEntityActiveBatchInCurrentTxn( } @Override - public @Nonnull Page listEntitiesInCurrentTxn( + public @Nonnull Page loadEntitiesInCurrentTxn( @Nonnull PolarisCallContext callCtx, long catalogId, long parentId, diff --git a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java index 6adb042acd..432e0001fd 100644 --- a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java +++ b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java @@ -36,7 +36,6 @@ import java.util.stream.Stream; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.AsyncTaskType; -import org.apache.polaris.core.entity.EntityNameLookupRecord; import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; @@ -118,33 +117,18 @@ protected void testCreateEntities() { .extracting(PolarisEntity::toCore) .containsExactly(PolarisEntity.toCore(task1), PolarisEntity.toCore(task2)); - List listedEntities = - metaStoreManager - .listEntities( - polarisTestMetaStoreManager.polarisCallContext, - null, - PolarisEntityType.TASK, - PolarisEntitySubType.NULL_SUBTYPE, - PageToken.readEverything()) - .getEntities(); + List listedEntities = + metaStoreManager.loadEntitiesAll( + polarisTestMetaStoreManager.polarisCallContext, + null, + PolarisEntityType.TASK, + PolarisEntitySubType.NULL_SUBTYPE, + TaskEntity::of); Assertions.assertThat(listedEntities) .isNotNull() .hasSize(2) - .containsExactly( - new EntityNameLookupRecord( - task1.getCatalogId(), - task1.getId(), - task1.getParentId(), - task1.getName(), - task1.getTypeCode(), - task1.getSubTypeCode()), - new EntityNameLookupRecord( - task2.getCatalogId(), - task2.getId(), - task2.getParentId(), - task2.getName(), - task2.getTypeCode(), - task2.getSubTypeCode())); + .extracting(PolarisEntity::toCore) + .containsExactly(PolarisEntity.toCore(task1), PolarisEntity.toCore(task2)); } @Test diff --git a/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java b/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java index e817b24047..3b34ce6503 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java @@ -106,7 +106,6 @@ import org.apache.polaris.core.persistence.dao.entity.EntityResult; import org.apache.polaris.core.persistence.dao.entity.LoadGrantsResult; import org.apache.polaris.core.persistence.dao.entity.PrivilegeResult; -import org.apache.polaris.core.persistence.pagination.PageToken; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest; import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory; import org.apache.polaris.core.persistence.resolver.ResolverPath; @@ -208,43 +207,6 @@ private Optional findCatalogRoleByName(String catalogName, St .map(path -> CatalogRoleEntity.of(path.getRawLeafEntity())); } - private Stream loadEntities( - @Nonnull PolarisEntityType entityType, - @Nonnull PolarisEntitySubType entitySubType, - @Nullable PolarisEntity catalogEntity, - @Nonnull Function transformer) { - List catalogPath; - long catalogId; - if (catalogEntity == null) { - catalogPath = null; - catalogId = 0; - } else { - catalogPath = PolarisEntity.toCoreList(List.of(catalogEntity)); - catalogId = catalogEntity.getId(); - } - // TODO: add loadEntities method to PolarisMetaStoreManager - // loadEntity may return null due to multiple non-atomic API calls to the persistence layer. - // Specifically, this can happen when a PolarisEntity is returned by listEntities, but cannot be - // loaded afterward because it was purged by another process before it could be loaded. - return metaStoreManager - .listEntities( - getCurrentPolarisContext(), - catalogPath, - entityType, - entitySubType, - PageToken.readEverything()) - .getEntities() - .stream() - .map( - nameAndId -> - metaStoreManager.loadEntity( - getCurrentPolarisContext(), catalogId, nameAndId.getId(), nameAndId.getType())) - .map(PolarisEntity::of) - .filter(Objects::nonNull) - .map(transformer) - .filter(Objects::nonNull); - } - private void authorizeBasicRootOperationOrThrow(PolarisAuthorizableOperation op) { resolutionManifest = resolutionManifestFactory.createResolutionManifest( @@ -983,8 +945,14 @@ public List listCatalogs() { /** List all catalogs without checking for permission. */ private Stream listCatalogsUnsafe() { - return loadEntities( - PolarisEntityType.CATALOG, PolarisEntitySubType.ANY_SUBTYPE, null, CatalogEntity::of); + return metaStoreManager + .loadEntitiesAll( + getCurrentPolarisContext(), + null, + PolarisEntityType.CATALOG, + PolarisEntitySubType.ANY_SUBTYPE, + CatalogEntity::of) + .stream(); } public PrincipalWithCredentials createPrincipal(PolarisEntity entity) { @@ -1152,11 +1120,14 @@ public List listPrincipals() { PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_PRINCIPALS; authorizeBasicRootOperationOrThrow(op); - return loadEntities( + return metaStoreManager + .loadEntitiesAll( + getCurrentPolarisContext(), + null, PolarisEntityType.PRINCIPAL, PolarisEntitySubType.NULL_SUBTYPE, - null, PrincipalEntity::of) + .stream() .map(PrincipalEntity::asPrincipal) .toList(); } @@ -1257,11 +1228,14 @@ public List listPrincipalRoles() { PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_PRINCIPAL_ROLES; authorizeBasicRootOperationOrThrow(op); - return loadEntities( + return metaStoreManager + .loadEntitiesAll( + getCurrentPolarisContext(), + null, PolarisEntityType.PRINCIPAL_ROLE, PolarisEntitySubType.NULL_SUBTYPE, - null, PrincipalRoleEntity::of) + .stream() .map(PrincipalRoleEntity::asPrincipalRole) .toList(); } @@ -1381,11 +1355,15 @@ public List listCatalogRoles(String catalogName) { PolarisEntity catalogEntity = findCatalogByName(catalogName) .orElseThrow(() -> new NotFoundException("Parent catalog %s not found", catalogName)); - return loadEntities( + List catalogPath = PolarisEntity.toCoreList(List.of(catalogEntity)); + return metaStoreManager + .loadEntitiesAll( + getCurrentPolarisContext(), + catalogPath, PolarisEntityType.CATALOG_ROLE, PolarisEntitySubType.NULL_SUBTYPE, - catalogEntity, CatalogRoleEntity::of) + .stream() .map(CatalogRoleEntity::asCatalogRole) .toList(); } diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index 7eec03595f..2493e9b48c 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -1118,8 +1118,8 @@ private void validateNoLocationO ListEntitiesResult siblingNamespacesResult = getMetaStoreManager() .listEntities( - callContext.getPolarisCallContext(), - parentPath.stream().map(PolarisEntity::toCore).collect(Collectors.toList()), + getCurrentPolarisContext(), + PolarisEntity.toCoreList(parentPath), PolarisEntityType.NAMESPACE, PolarisEntitySubType.ANY_SUBTYPE, PageToken.readEverything()); @@ -1135,10 +1135,8 @@ private void validateNoLocationO ListEntitiesResult siblingTablesResult = getMetaStoreManager() .listEntities( - callContext.getPolarisCallContext(), - parentPath.stream() - .map(PolarisEntity::toCore) - .collect(Collectors.toList()), + getCurrentPolarisContext(), + PolarisEntity.toCoreList(parentPath), PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ANY_SUBTYPE, PageToken.readEverything()); diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index e0edebfc64..09a706c85c 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -50,7 +50,6 @@ import org.apache.polaris.core.persistence.PolicyMappingAlreadyExistsException; import org.apache.polaris.core.persistence.dao.entity.EntityResult; import org.apache.polaris.core.persistence.dao.entity.LoadPolicyMappingsResult; -import org.apache.polaris.core.persistence.pagination.PageToken; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; import org.apache.polaris.core.policy.PolicyEntity; import org.apache.polaris.core.policy.PolicyType; @@ -162,34 +161,15 @@ public List listPolicies(Namespace namespace, PolicyType polic } List catalogPath = resolvedEntities.getRawFullPath(); - List policyEntities = - metaStoreManager - .listEntities( - callContext.getPolarisCallContext(), - PolarisEntity.toCoreList(catalogPath), - PolarisEntityType.POLICY, - PolarisEntitySubType.NULL_SUBTYPE, - PageToken.readEverything()) - .getEntities() - .stream() - .map( - polarisEntityActiveRecord -> - PolicyEntity.of( - metaStoreManager - .loadEntity( - callContext.getPolarisCallContext(), - polarisEntityActiveRecord.getCatalogId(), - polarisEntityActiveRecord.getId(), - polarisEntityActiveRecord.getType()) - .getEntity())) - .filter( - policyEntity -> policyType == null || policyEntity.getPolicyType() == policyType) - .toList(); - - List entities = - policyEntities.stream().map(PolarisEntity::nameAndId).toList(); - - return entities.stream() + return metaStoreManager + .loadEntitiesAll( + callContext.getPolarisCallContext(), + PolarisEntity.toCoreList(catalogPath), + PolarisEntityType.POLICY, + PolarisEntitySubType.NULL_SUBTYPE, + PolicyEntity::of) + .stream() + .filter(policyEntity -> policyType == null || policyEntity.getPolicyType() == policyType) .map( entity -> PolicyIdentifier.builder() diff --git a/runtime/service/src/test/java/org/apache/polaris/service/admin/ManagementServiceTest.java b/runtime/service/src/test/java/org/apache/polaris/service/admin/ManagementServiceTest.java index 422af05cf4..d5a8b9c860 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/admin/ManagementServiceTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/admin/ManagementServiceTest.java @@ -49,7 +49,6 @@ import org.apache.polaris.core.entity.PrincipalRoleEntity; import org.apache.polaris.core.persistence.MetaStoreManagerFactory; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; -import org.apache.polaris.core.persistence.dao.entity.BaseResult; import org.apache.polaris.core.persistence.dao.entity.CreateCatalogResult; import org.apache.polaris.core.persistence.dao.entity.EntityResult; import org.apache.polaris.core.secrets.UnsafeInMemorySecretsManager; @@ -57,7 +56,6 @@ import org.apache.polaris.service.config.ReservedProperties; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.mockito.Mockito; public class ManagementServiceTest { private TestServices services; @@ -248,10 +246,9 @@ public void testCannotAssignFederatedEntities() { .isInstanceOf(ValidationException.class); } - /** Simulates the case when a catalog is dropped after being found while listing all catalogs. */ @Test - public void testCatalogNotReturnedWhenDeletedAfterListBeforeGet() { - PolarisMetaStoreManager metaStoreManager = Mockito.spy(setupMetaStoreManager()); + public void testCanListCatalogs() { + PolarisMetaStoreManager metaStoreManager = setupMetaStoreManager(); PolarisCallContext callContext = services.newCallContext(); PolarisAdminService polarisAdminService = setupPolarisAdminService(metaStoreManager, callContext); @@ -267,6 +264,8 @@ public void testCatalogNotReturnedWhenDeletedAfterListBeforeGet() { PolarisEntityConstants.getRootEntityId(), "my-catalog-1"), List.of()); + assertThat(catalog1.isSuccess()).isTrue(); + CreateCatalogResult catalog2 = metaStoreManager.createCatalog( callContext, @@ -278,20 +277,12 @@ public void testCatalogNotReturnedWhenDeletedAfterListBeforeGet() { PolarisEntityConstants.getRootEntityId(), "my-catalog-2"), List.of()); - - Mockito.doAnswer( - invocation -> { - Object entityId = invocation.getArgument(2); - if (entityId.equals(catalog1.getCatalog().getId())) { - return new EntityResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, ""); - } - return invocation.callRealMethod(); - }) - .when(metaStoreManager) - .loadEntity(Mockito.any(), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); + assertThat(catalog2.isSuccess()).isTrue(); List catalogs = polarisAdminService.listCatalogs(); - assertThat(catalogs.size()).isEqualTo(1); - assertThat(catalogs.getFirst().getName()).isEqualTo(catalog2.getCatalog().getName()); + assertThat(catalogs.size()).isEqualTo(2); + assertThat(catalogs) + .extracting(Catalog::getName) + .containsExactlyInAnyOrder("my-catalog-1", "my-catalog-2"); } } From da437480bf0d1fe6a1a8d80a98a7e6a97dca702e Mon Sep 17 00:00:00 2001 From: Christopher Lambert <1204398+XN137@users.noreply.github.com> Date: Sat, 16 Aug 2025 09:34:11 +0200 Subject: [PATCH 2/4] review: remove filter + transformer params --- .../AtomicOperationMetaStoreManager.java | 9 ++---- .../persistence/PolarisMetaStoreManager.java | 32 ++++--------------- .../TransactionWorkspaceMetaStoreManager.java | 6 +--- .../TransactionalMetaStoreManagerImpl.java | 26 ++++----------- .../TransactionalPersistence.java | 2 +- .../BasePolarisMetaStoreManagerTest.java | 5 ++- .../service/admin/PolarisAdminService.java | 18 +++++------ .../service/catalog/policy/PolicyCatalog.java | 4 +-- 8 files changed, 31 insertions(+), 71 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java index c31efd6692..7d7a26c486 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java @@ -31,7 +31,6 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; -import java.util.function.Predicate; import java.util.stream.Collectors; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.AsyncTaskType; @@ -723,13 +722,11 @@ private void revokeGrantRecord( /** {@inheritDoc} */ @Override - public @Nonnull Page loadEntities( + public @Nonnull Page loadEntities( @Nonnull PolarisCallContext callCtx, @Nullable List catalogPath, @Nonnull PolarisEntityType entityType, @Nonnull PolarisEntitySubType entitySubType, - @Nonnull Predicate entityFilter, - @Nonnull Function transformer, @Nonnull PageToken pageToken) { // get meta store we should be using BasePersistence ms = callCtx.getMetaStore(); @@ -754,8 +751,8 @@ private void revokeGrantRecord( parentId, entityType, entitySubType, - entityFilter, - transformer, + entity -> true, + Function.identity(), pageToken); } 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 206d3e3ae4..bcae48687c 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 @@ -23,8 +23,6 @@ import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.function.Function; -import java.util.function.Predicate; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.auth.PolarisGrantManager; import org.apache.polaris.core.auth.PolarisSecretsManager; @@ -133,55 +131,39 @@ ListEntitiesResult listEntities( @Nonnull PageToken pageToken); /** - * Load entities where some predicate returns true and transform the entities with a function + * Load entities with pagination * * @param callCtx call context * @param catalogPath path inside a catalog. If null or empty, the entities to list are top-level, * like catalogs * @param entityType type of entities to list * @param entitySubType subType of entities to list (or ANY_SUBTYPE) - * @param entityFilter the filter to be applied to each entity. Only entities where the predicate - * returns true are returned in the list - * @param transformer the transformation function applied to the {@link PolarisBaseEntity} before - * returning - * @return the paged list of entities for which the predicate returns true + * @return the paged list of entities */ @Nonnull - Page loadEntities( + Page loadEntities( @Nonnull PolarisCallContext callCtx, @Nullable List catalogPath, @Nonnull PolarisEntityType entityType, @Nonnull PolarisEntitySubType entitySubType, - @Nonnull Predicate entityFilter, - @Nonnull Function transformer, @Nonnull PageToken pageToken); /** - * Load all entities and transform the entities with a function into an unpaged list + * Load all entities into an unpaged list * * @param callCtx call context * @param catalogPath path inside a catalog. If null or empty, the entities to list are top-level, * like catalogs * @param entityType type of entities to list * @param entitySubType subType of entities to list (or ANY_SUBTYPE) - * @param transformer the transformation function applied to the {@link PolarisBaseEntity} before - * returning * @return the full list of entities */ - default @Nonnull List loadEntitiesAll( + default @Nonnull List loadEntitiesAll( @Nonnull PolarisCallContext callCtx, @Nullable List catalogPath, @Nonnull PolarisEntityType entityType, - @Nonnull PolarisEntitySubType entitySubType, - @Nonnull Function transformer) { - return loadEntities( - callCtx, - catalogPath, - entityType, - entitySubType, - e -> true, - transformer, - PageToken.readEverything()) + @Nonnull PolarisEntitySubType entitySubType) { + return loadEntities(callCtx, catalogPath, entityType, entitySubType, PageToken.readEverything()) .items(); } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java index eee3a34b27..2671ad98b9 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java @@ -26,8 +26,6 @@ import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.function.Function; -import java.util.function.Predicate; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; import org.apache.polaris.core.entity.LocationBasedEntity; @@ -133,13 +131,11 @@ public EntityResult readEntityByName( } @Override - public @Nonnull Page loadEntities( + public @Nonnull Page loadEntities( @Nonnull PolarisCallContext callCtx, @Nullable List catalogPath, @Nonnull PolarisEntityType entityType, @Nonnull PolarisEntitySubType entitySubType, - @Nonnull Predicate entityFilter, - @Nonnull Function transformer, @Nonnull PageToken pageToken) { callCtx.getDiagServices().fail("illegal_method_in_transaction_workspace", "loadEntities"); return null; diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java index b40aed0838..c187a6375a 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java @@ -30,7 +30,6 @@ import java.util.Optional; import java.util.Set; import java.util.function.Function; -import java.util.function.Predicate; import java.util.stream.Collectors; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; @@ -722,16 +721,14 @@ private void bootstrapPolarisService( /** * See {@link PolarisMetaStoreManager#loadEntities(PolarisCallContext, List, PolarisEntityType, - * PolarisEntitySubType, Predicate, Function, PageToken)} + * PolarisEntitySubType, PageToken)} */ - private @Nonnull Page loadEntities( + private @Nonnull Page loadEntities( @Nonnull PolarisCallContext callCtx, @Nonnull TransactionalPersistence ms, @Nullable List catalogPath, @Nonnull PolarisEntityType entityType, @Nonnull PolarisEntitySubType entitySubType, - @Nonnull Predicate entityFilter, - @Nonnull Function transformer, @Nonnull PageToken pageToken) { // first resolve again the catalogPath to that entity PolarisEntityResolver resolver = new PolarisEntityResolver(callCtx, ms, catalogPath); @@ -748,20 +745,18 @@ private void bootstrapPolarisService( resolver.getParentId(), entityType, entitySubType, - entityFilter, - transformer, + entity -> true, + Function.identity(), pageToken); } /** {@inheritDoc} */ @Override - public @Nonnull Page loadEntities( + public @Nonnull Page loadEntities( @Nonnull PolarisCallContext callCtx, @Nullable List catalogPath, @Nonnull PolarisEntityType entityType, @Nonnull PolarisEntitySubType entitySubType, - @Nonnull Predicate entityFilter, - @Nonnull Function transformer, @Nonnull PageToken pageToken) { // get meta store we should be using TransactionalPersistence ms = ((TransactionalPersistence) callCtx.getMetaStore()); @@ -769,16 +764,7 @@ private void bootstrapPolarisService( // run operation in a read transaction return ms.runInReadTransaction( callCtx, - () -> - loadEntities( - callCtx, - ms, - catalogPath, - entityType, - entitySubType, - entityFilter, - transformer, - pageToken)); + () -> loadEntities(callCtx, ms, catalogPath, entityType, entitySubType, pageToken)); } /** {@link #createPrincipal(PolarisCallContext, PolarisBaseEntity)} */ diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java index eb3371c66c..3802908b82 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java @@ -225,7 +225,7 @@ List lookupEntityVersionsInCurrentTxn( pageToken); } - /** See {@link org.apache.polaris.core.persistence.BasePersistence#listEntities} */ + /** See {@link org.apache.polaris.core.persistence.BasePersistence#loadEntities} */ @Nonnull Page loadEntitiesInCurrentTxn( @Nonnull PolarisCallContext callCtx, diff --git a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java index 432e0001fd..336b6b5d03 100644 --- a/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java +++ b/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java @@ -117,13 +117,12 @@ protected void testCreateEntities() { .extracting(PolarisEntity::toCore) .containsExactly(PolarisEntity.toCore(task1), PolarisEntity.toCore(task2)); - List listedEntities = + List listedEntities = metaStoreManager.loadEntitiesAll( polarisTestMetaStoreManager.polarisCallContext, null, PolarisEntityType.TASK, - PolarisEntitySubType.NULL_SUBTYPE, - TaskEntity::of); + PolarisEntitySubType.NULL_SUBTYPE); Assertions.assertThat(listedEntities) .isNotNull() .hasSize(2) diff --git a/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java b/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java index 3b34ce6503..9462e13d6a 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java @@ -950,9 +950,9 @@ private Stream listCatalogsUnsafe() { getCurrentPolarisContext(), null, PolarisEntityType.CATALOG, - PolarisEntitySubType.ANY_SUBTYPE, - CatalogEntity::of) - .stream(); + PolarisEntitySubType.ANY_SUBTYPE) + .stream() + .map(CatalogEntity::of); } public PrincipalWithCredentials createPrincipal(PolarisEntity entity) { @@ -1125,9 +1125,9 @@ public List listPrincipals() { getCurrentPolarisContext(), null, PolarisEntityType.PRINCIPAL, - PolarisEntitySubType.NULL_SUBTYPE, - PrincipalEntity::of) + PolarisEntitySubType.NULL_SUBTYPE) .stream() + .map(PrincipalEntity::of) .map(PrincipalEntity::asPrincipal) .toList(); } @@ -1233,9 +1233,9 @@ public List listPrincipalRoles() { getCurrentPolarisContext(), null, PolarisEntityType.PRINCIPAL_ROLE, - PolarisEntitySubType.NULL_SUBTYPE, - PrincipalRoleEntity::of) + PolarisEntitySubType.NULL_SUBTYPE) .stream() + .map(PrincipalRoleEntity::of) .map(PrincipalRoleEntity::asPrincipalRole) .toList(); } @@ -1361,9 +1361,9 @@ public List listCatalogRoles(String catalogName) { getCurrentPolarisContext(), catalogPath, PolarisEntityType.CATALOG_ROLE, - PolarisEntitySubType.NULL_SUBTYPE, - CatalogRoleEntity::of) + PolarisEntitySubType.NULL_SUBTYPE) .stream() + .map(CatalogRoleEntity::of) .map(CatalogRoleEntity::asCatalogRole) .toList(); } diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index 09a706c85c..361dae5cba 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -166,9 +166,9 @@ public List listPolicies(Namespace namespace, PolicyType polic callContext.getPolarisCallContext(), PolarisEntity.toCoreList(catalogPath), PolarisEntityType.POLICY, - PolarisEntitySubType.NULL_SUBTYPE, - PolicyEntity::of) + PolarisEntitySubType.NULL_SUBTYPE) .stream() + .map(PolicyEntity::of) .filter(policyEntity -> policyType == null || policyEntity.getPolicyType() == policyType) .map( entity -> From 06c52ae15ccd92fdea7afd02916a9da7977485c1 Mon Sep 17 00:00:00 2001 From: Christopher Lambert <1204398+XN137@users.noreply.github.com> Date: Sat, 16 Aug 2025 10:07:52 +0200 Subject: [PATCH 3/4] review: add TODO in listPolicies --- .../org/apache/polaris/service/catalog/policy/PolicyCatalog.java | 1 + 1 file changed, 1 insertion(+) diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index 361dae5cba..6cc8ba35d9 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -161,6 +161,7 @@ public List listPolicies(Namespace namespace, PolicyType polic } List catalogPath = resolvedEntities.getRawFullPath(); + // TODO: when the "policyType" filter is null we should only call "listEntities" instead return metaStoreManager .loadEntitiesAll( callContext.getPolarisCallContext(), From 597e81b9cd3b7fef9b2fc03917a42007a54b2017 Mon Sep 17 00:00:00 2001 From: Christopher Lambert <1204398+XN137@users.noreply.github.com> Date: Wed, 20 Aug 2025 11:09:17 +0200 Subject: [PATCH 4/4] review: improve javadoc --- .../core/persistence/BasePersistence.java | 10 ++++++---- .../persistence/PolarisMetaStoreManager.java | 16 ++++++++++------ 2 files changed, 16 insertions(+), 10 deletions(-) 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 76ceb0520d..6134e5af6e 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 @@ -270,7 +270,8 @@ List lookupEntityVersions( @Nonnull PolarisCallContext callCtx, List entityIds); /** - * List all entities of the specified type which are child entities of the specified parent + * List lightweight information of entities matching the given criteria with pagination. If all + * properties of the entity are required,use {@link #loadEntities} instead. * * @param callCtx call context * @param catalogId catalog id for that entity, NULL_ID if the entity is top-level @@ -278,7 +279,7 @@ List lookupEntityVersions( * @param entityType type of entities to list * @param entitySubType subType of entities to list (or ANY_SUBTYPE) * @param pageToken the token to start listing after - * @return the list of entities for the specified list operation + * @return the paged list of matching entities */ @Nonnull Page listEntities( @@ -290,7 +291,8 @@ Page listEntities( @Nonnull PageToken pageToken); /** - * Load entities where some predicate returns true and transform the entities with a function + * Load full entities matching the given criteria with pagination and transformation. If only the + * entity name/id/type is required, use {@link #listEntities} instead. * * @param callCtx call context * @param catalogId catalog id for that entity, NULL_ID if the entity is top-level @@ -301,7 +303,7 @@ Page listEntities( * returns true are returned in the list * @param transformer the transformation function applied to the {@link PolarisBaseEntity} before * returning - * @return the list of entities for which the predicate returns true + * @return the paged list of matching entities after transformation */ @Nonnull Page loadEntities( 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 bcae48687c..ffe3ca2a26 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 @@ -111,8 +111,8 @@ EntityResult readEntityByName( @Nonnull String name); /** - * List all entities of the specified type under the specified catalogPath. If the catalogPath is - * null, listed entities will be top-level entities like catalogs. + * List lightweight information about entities matching the given criteria. If all properties of + * the entity are required,use {@link #loadEntities} instead. * * @param callCtx call context * @param catalogPath path inside a catalog. If null or empty, the entities to list are top-level, @@ -131,14 +131,16 @@ ListEntitiesResult listEntities( @Nonnull PageToken pageToken); /** - * Load entities with pagination + * Load full entities matching the given criteria with pagination. If only the entity name/id/type + * is required, use {@link #listEntities} instead. If no pagination is required, use {@link + * #loadEntitiesAll} instead. * * @param callCtx call context * @param catalogPath path inside a catalog. If null or empty, the entities to list are top-level, * like catalogs * @param entityType type of entities to list * @param entitySubType subType of entities to list (or ANY_SUBTYPE) - * @return the paged list of entities + * @return paged list of matching entities */ @Nonnull Page loadEntities( @@ -149,14 +151,16 @@ Page loadEntities( @Nonnull PageToken pageToken); /** - * Load all entities into an unpaged list + * Load full entities matching the given criteria into an unpaged list. If pagination is required + * use {@link #loadEntities} instead. If only the entity name/id/type is required, use {@link + * #listEntities} instead. * * @param callCtx call context * @param catalogPath path inside a catalog. If null or empty, the entities to list are top-level, * like catalogs * @param entityType type of entities to list * @param entitySubType subType of entities to list (or ANY_SUBTYPE) - * @return the full list of entities + * @return list of all matching entities */ default @Nonnull List loadEntitiesAll( @Nonnull PolarisCallContext callCtx,