dennishuo commented on code in PR #2262:
URL: https://github.com/apache/polaris/pull/2262#discussion_r2261954000
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -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,
Review Comment:
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.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java:
##########
@@ -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 ->
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
