Skip to content

Remove overloads of BasePersistence.listEntities #2262

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Remove overloads of BasePersistence.listEntities #2262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 0 additions & 39 deletions ...olaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.apache.polaris.extension.persistence.impl.eclipselink;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Predicates;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import jakarta.persistence.EntityManager;
Expand Down Expand Up @@ -424,44 +423,6 @@ public List<EntityNameLookupRecord> lookupEntityActiveBatchInCurrentTxn(
.collect(Collectors.toList());
}

/** {@inheritDoc} */
@Override
public @Nonnull Page<EntityNameLookupRecord> listEntitiesInCurrentTxn(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull PageToken pageToken) {
return this.listEntitiesInCurrentTxn(
callCtx, catalogId, parentId, entityType, Predicates.alwaysTrue(), pageToken);
}

@Override
public @Nonnull Page<EntityNameLookupRecord> listEntitiesInCurrentTxn(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull Predicate<PolarisBaseEntity> entityFilter,
@Nonnull PageToken pageToken) {
// full range scan under the parent for that type
return this.listEntitiesInCurrentTxn(
callCtx,
catalogId,
parentId,
entityType,
entityFilter,
entity ->
new EntityNameLookupRecord(
entity.getCatalogId(),
entity.getId(),
entity.getParentId(),
entity.getName(),
entity.getTypeCode(),
entity.getSubTypeCode()),
pageToken);
}

@Override
public @Nonnull <T> Page<T> listEntitiesInCurrentTxn(
@Nonnull PolarisCallContext callCtx,
Expand Down
3 changes: 2 additions & 1 deletion ...va/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ void writeToEntitiesActive(EntityManager session, PolarisBaseEntity entity) {

ModelEntityActive model = lookupEntityActive(session, new PolarisEntitiesActiveKey(entity));
if (model == null) {
session.persist(ModelEntityActive.fromEntityActive(new EntityNameLookupRecord(entity)));
session.persist(
ModelEntityActive.fromEntityActive(EntityNameLookupRecord.fromEntity(entity)));
}
}

Expand Down
40 changes: 1 addition & 39 deletions ...src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import java.util.function.Predicate;
import java.util.stream.Collectors;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.entity.EntityNameLookupRecord;
import org.apache.polaris.core.entity.LocationBasedEntity;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisChangeTrackingVersions;
Expand Down Expand Up @@ -420,50 +419,13 @@ public List<PolarisChangeTrackingVersions> lookupEntityVersions(
.collect(Collectors.toList());
}

@Nonnull
@Override
public Page<EntityNameLookupRecord> listEntities(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull PageToken pageToken) {
return listEntities(
callCtx,
catalogId,
parentId,
entityType,
entity -> true,
EntityNameLookupRecord::new,
pageToken);
}

@Nonnull
@Override
public Page<EntityNameLookupRecord> listEntities(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull Predicate<PolarisBaseEntity> entityFilter,
@Nonnull PageToken pageToken) {
return listEntities(
callCtx,
catalogId,
parentId,
entityType,
entityFilter,
EntityNameLookupRecord::new,
Copy link
Contributor

Choose a reason for hiding this comment

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

This was an unfortunate inefficiency that accidentally slipped through probably because the TreeMap impl gets away with it due to everything being in-memory. The "basic" version should never have delegated to the "filter" or "transformer" version, and indeed even the filter version probably shouldn't have just delegated to the "transformer" version either.

In practice, at the very least the "basic" version (5-arg version) should have had its own underlying query which only retrieved the columns used in EntityNameLookupRecord.

It's a pretty huge inefficiency (borderline bug) right now for "pure name listing" to have to select all columns from the database only to throw away all the big columns.

pageToken);
}

@Nonnull
@Override
public <T> Page<T> listEntities(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
PolarisEntityType entityType,
@Nonnull PolarisEntityType entityType,
@Nonnull Predicate<PolarisBaseEntity> entityFilter,
@Nonnull Function<PolarisBaseEntity, T> transformer,
@Nonnull PageToken pageToken) {
Expand Down
18 changes: 9 additions & 9 deletions polaris-core/src/main/java/org/apache/polaris/core/entity/EntityNameLookupRecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,14 @@ public EntityNameLookupRecord(
this.subTypeCode = subTypeCode;
}

/** Constructor to create the object with provided entity */
public EntityNameLookupRecord(PolarisBaseEntity entity) {
this.catalogId = entity.getCatalogId();
this.id = entity.getId();
this.parentId = entity.getParentId();
this.typeCode = entity.getTypeCode();
this.name = entity.getName();
this.subTypeCode = entity.getSubTypeCode();
public static EntityNameLookupRecord fromEntity(PolarisBaseEntity entity) {
Copy link

Choose a reason for hiding this comment

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

what is the motivation for this change here? if no particular reason, can we keep the original constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine to add a new static method like this. I'd suggest to deprecate the constructor first and remove it later if we don't need it.

return new EntityNameLookupRecord(
entity.getCatalogId(),
entity.getId(),
entity.getParentId(),
entity.getName(),
entity.getTypeCode(),
entity.getSubTypeCode());
}

@Override
Expand All @@ -119,7 +119,7 @@ public int hashCode() {

@Override
public String toString() {
return "PolarisEntitiesActiveRecord{"
return "EntityNameLookupRecord{"
+ "catalogId="
+ catalogId
+ ", id="
Expand Down
9 changes: 8 additions & 1 deletion ...re/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,14 @@ private void revokeGrantRecord(
: entity -> true;

Page<EntityNameLookupRecord> resultPage =
ms.listEntities(callCtx, catalogId, parentId, entityType, filter, pageToken);
ms.listEntities(
callCtx,
catalogId,
parentId,
entityType,
filter,
EntityNameLookupRecord::fromEntity,
pageToken);

// TODO: Use post-validation to enforce consistent view against catalogPath. In the
// meantime, happens-before ordering semantics aren't guaranteed during high-concurrency
Expand Down
41 changes: 1 addition & 40 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ default EntityNameLookupRecord lookupEntityIdAndSubTypeByName(
if (baseEntity == null) {
return null;
}
return new EntityNameLookupRecord(baseEntity);
return EntityNameLookupRecord.fromEntity(baseEntity);
}

/**
Expand All @@ -268,45 +268,6 @@ List<PolarisBaseEntity> lookupEntities(
List<PolarisChangeTrackingVersions> lookupEntityVersions(
@Nonnull PolarisCallContext callCtx, List<PolarisEntityId> entityIds);

/**
* List all entities of the specified type which are child entities of the specified parent
*
* @param callCtx call context
* @param catalogId catalog id for that entity, NULL_ID if the entity is top-level
* @param parentId id of the parent, can be the special 0 value representing the root entity
* @param entityType type of entities to list
* @param pageToken the token to start listing after
* @return the list of entities for the specified list operation
*/
@Nonnull
Page<EntityNameLookupRecord> listEntities(
Copy link
Contributor

@flyrain flyrain Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'd suggest a dev ML discussion or vote for interface change in this class. cc @dennishuo cc @singhpk234

Copy link
Member

Choose a reason for hiding this comment

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

This particular type is actually not even used by any production code outside of a particular persistence implementation.

If a particular implementation needs more functions, those implementations are free to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you help me understand what particular concern one might have?

the innermost methods that support all the parameters are still there (so not that hard to switch to if backward-compatibility is the concern)

Copy link
Contributor

Choose a reason for hiding this comment

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

Because BasePersistence is a core persistence interface, any change—no matter how small—deserves a note on the dev mailing list so everyone stays in the loop.

@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull PageToken pageToken);

/**
* List entities where some predicate returns true
*
* @param callCtx call context
* @param catalogId catalog id for that entity, NULL_ID if the entity is top-level
* @param parentId id of the parent, can be the special 0 value representing the root entity
* @param entityType type of entities to list
* @param entityFilter the filter to be applied to each entity. Only entities where the predicate
* returns true are returned in the list
* @param pageToken the token to start listing after
* @return the list of entities for which the predicate returns true
*/
@Nonnull
Page<EntityNameLookupRecord> listEntities(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull Predicate<PolarisBaseEntity> entityFilter,
@Nonnull PageToken pageToken);

/**
* List entities where some predicate returns true and transform the entities with a function
*
Expand Down
31 changes: 0 additions & 31 deletions ...a/org/apache/polaris/core/persistence/transactional/AbstractTransactionalPersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,37 +351,6 @@ public List<PolarisChangeTrackingVersions> lookupEntityVersions(
callCtx, () -> this.lookupEntityVersionsInCurrentTxn(callCtx, entityIds));
}

/** {@inheritDoc} */
@Override
@Nonnull
public Page<EntityNameLookupRecord> listEntities(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull PageToken pageToken) {
return runInReadTransaction(
callCtx,
() -> this.listEntitiesInCurrentTxn(callCtx, catalogId, parentId, entityType, pageToken));
}

/** {@inheritDoc} */
@Override
@Nonnull
public Page<EntityNameLookupRecord> listEntities(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull Predicate<PolarisBaseEntity> entityFilter,
@Nonnull PageToken pageToken) {
return runInReadTransaction(
callCtx,
() ->
this.listEntitiesInCurrentTxn(
callCtx, catalogId, parentId, entityType, entityFilter, pageToken));
}

/** {@inheritDoc} */
@Override
@Nonnull
Expand Down
1 change: 1 addition & 0 deletions .../org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,7 @@ private void bootstrapPolarisService(
resolver.getParentId(),
entityType,
filter,
EntityNameLookupRecord::fromEntity,
pageToken);

// done
Expand Down
19 changes: 0 additions & 19 deletions ...main/java/org/apache/polaris/core/persistence/transactional/TransactionalPersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,25 +201,6 @@ List<PolarisBaseEntity> lookupEntitiesInCurrentTxn(
List<PolarisChangeTrackingVersions> lookupEntityVersionsInCurrentTxn(
@Nonnull PolarisCallContext callCtx, List<PolarisEntityId> entityIds);

/** See {@link org.apache.polaris.core.persistence.BasePersistence#listEntities} */
@Nonnull
Page<EntityNameLookupRecord> listEntitiesInCurrentTxn(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull PageToken pageToken);

/** See {@link org.apache.polaris.core.persistence.BasePersistence#listEntities} */
@Nonnull
Page<EntityNameLookupRecord> listEntitiesInCurrentTxn(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull Predicate<PolarisBaseEntity> entityFilter,
@Nonnull PageToken pageToken);

/** See {@link org.apache.polaris.core.persistence.BasePersistence#listEntities} */
@Nonnull
<T> Page<T> listEntitiesInCurrentTxn(
Expand Down
49 changes: 1 addition & 48 deletions ...rg/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/
package org.apache.polaris.core.persistence.transactional;

import com.google.common.base.Predicates;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.Comparator;
Expand Down Expand Up @@ -288,15 +287,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 : EntityNameLookupRecord.fromEntity(entity);
}

/** {@inheritDoc} */
Expand All @@ -311,44 +302,6 @@ public List<EntityNameLookupRecord> lookupEntityActiveBatchInCurrentTxn(
.collect(Collectors.toList());
}

/** {@inheritDoc} */
@Override
public @Nonnull Page<EntityNameLookupRecord> listEntitiesInCurrentTxn(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull PageToken pageToken) {
return this.listEntitiesInCurrentTxn(
callCtx, catalogId, parentId, entityType, Predicates.alwaysTrue(), pageToken);
}

@Override
public @Nonnull Page<EntityNameLookupRecord> listEntitiesInCurrentTxn(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
@Nonnull Predicate<PolarisBaseEntity> entityFilter,
@Nonnull PageToken pageToken) {
// full range scan under the parent for that type
return this.listEntitiesInCurrentTxn(
callCtx,
catalogId,
parentId,
entityType,
entityFilter,
entity ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so we shouldn't have been lazy with properly expressing the distinctions in the behaviors of the different methods here in TreeMapTransactionalPersistenceImpl because it hid the real behavioral nuance since inmemory operations are too cheap.

In general the entitesActive is not a "covering index" for the entire PolarisBaseEntity, so operations that require the whole entity are not in the same category as those which only require EntityNameLookupRecord.

new EntityNameLookupRecord(
entity.getCatalogId(),
entity.getId(),
entity.getParentId(),
entity.getName(),
entity.getTypeCode(),
entity.getSubTypeCode()),
pageToken);
}

@Override
public @Nonnull <T> Page<T> listEntitiesInCurrentTxn(
@Nonnull PolarisCallContext callCtx,
Expand Down
15 changes: 1 addition & 14 deletions ...estFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,20 +130,7 @@ protected void testCreateEntities() {
.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()));
EntityNameLookupRecord.fromEntity(task1), EntityNameLookupRecord.fromEntity(task2));
}

@Test
Expand Down