dennishuo commented on code in PR #2508:
URL: https://github.com/apache/polaris/pull/2508#discussion_r2388910056
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -416,6 +419,38 @@ ResolvedEntityResult loadResolvedEntityByName(
@Nonnull PolarisEntityType entityType,
@Nonnull String entityName);
+ /**
+ * Load a batch of resolved entities given their {@link
EntityNameLookupRecord}. Will return an
+ * empty list if the input list is empty. Order in that returned list is the
same as the input
+ * list. Some elements might be NULL if the entity has been dropped.
+ *
+ * @param callCtx call context
+ * @param entityLookupRecords the list of entity lookup records to load
+ * @return a non-null list of entities corresponding to the lookup keys.
Some elements might be
+ * NULL if the entity has been dropped.
+ */
+ @Nonnull
+ ResolvedEntitiesResult loadResolvedEntities(
Review Comment:
The existence of this method may cause confusion since the single-lookup
methods have both `ByName` and `ById` variations, and an
`EntityNameLookupRecord` happens to have the name in it, and yet it looks like
the actual key difference between the two methods is whether we have
`EntityType` per item.
I'm not sure if we already use `EntityNameLookupRecord` as an *input*
argument anywhere else, but generally since it was kind of structured as an
*output* argument before it seems it becomes ambiguous as an input argument.
And at first glance the unittests seems to imply that the lookup would be
"by name":
@ParameterizedTest
@ValueSource(strings = {"id", "name"})
....
if (loadType.equals("id")) {
// Create entity ID list with the updated entity
List<PolarisEntityId> entityIds =
List.of(getPolarisEntityId(T6v2));
// Call batch load - this should detect the stale version and
reload
results =
cache.getOrLoadResolvedEntities(this.callCtx,
PolarisEntityType.TABLE_LIKE, entityIds);
} else {
results =
cache.getOrLoadResolvedEntities(this.callCtx, List.of(new
EntityNameLookupRecord(T6v2)));
}
To match convention with the single-item lookups can we rename these methods
to say `loadResolvedEntitiesById`?
And if the difference is really just whether we pass in a per-entity
EntityType, I think even parallel Lists (`List<EntityId>, List<EntityType>`
would be better than reusing `EntityNameLookupRecord` just for its `catalogId,
entityId, entityType`.
Alternatively, cleanest would be just having one interface entirely with
`List<EntityIdAndType>` as the input argument.
--
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]
