dennishuo commented on code in PR #1112:
URL: https://github.com/apache/polaris/pull/1112#discussion_r1980690670
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1650,15 +1664,42 @@ public List<GrantResource>
listGrantsForCatalogRole(String catalogName, String c
* @param entitiesMap map of entities
* @param catalogId the id of the catalog of the entity we are looking for
* @param id id of the entity we are looking for
+ * @param entityType
* @return null if the entity does not exist
*/
private @Nullable PolarisBaseEntity getOrLoadEntity(
- @Nullable Map<Long, PolarisBaseEntity> entitiesMap, long catalogId, long
id) {
+ @Nullable Map<Long, PolarisBaseEntity> entitiesMap,
+ long catalogId,
+ long id,
+ PolarisEntityType entityType) {
return (entitiesMap == null)
- ? metaStoreManager.loadEntity(getCurrentPolarisContext(), catalogId,
id).getEntity()
+ ? metaStoreManager
+ .loadEntity(getCurrentPolarisContext(), catalogId, id, entityType)
+ .getEntity()
: entitiesMap.get(id);
}
+ private @Nullable PolarisBaseEntity getOrLoadEntityForGrant(
+ @Nullable Map<Long, PolarisBaseEntity> entitiesMap, PolarisGrantRecord
record) {
+ if (entitiesMap != null) {
+ return entitiesMap.get(record.getSecurableId());
+ }
+
+ for (PolarisEntityType type : PolarisEntityType.values()) {
Review Comment:
It's kind of nice that the default behavior here will already let the
persistence impls which happen not to use PolarisEntityType to short-circuit
and return the entity on the first lookup, but perhaps we should just
officially push-down the idea of "ANY"/"UNKNOWN" type to the lower layer and
make it the persistence impl's responsibility to perform the for-loop.
We could allow `NULL_TYPE` to indicate the persistence layer should fan-out
itself, or I guess we could add `ANY_TYPE` as `-1` the sme way
`PolarisEntitySubType` has `ANY_SUBTYPE(-1, null)`.
That would also enable impls which group some set of types into one place
and other sets of entities into another place to only have to fan-out over the
number of *groupings* rather than let the for-loop iterate over *all* possible
types.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -285,7 +285,11 @@ DropEntityResult dropEntityIfExists(
* @param entityId the id of the entity to load
*/
@Nonnull
- EntityResult loadEntity(@Nonnull PolarisCallContext callCtx, long
entityCatalogId, long entityId);
+ EntityResult loadEntity(
+ @Nonnull PolarisCallContext callCtx,
+ long entityCatalogId,
+ long entityId,
+ @Nonnull PolarisEntityType entityType);
Review Comment:
We should document in the javadoc whether this is intended to only be a
"hint" or whether there's expectations that the type is enforced strictly. In
particular, since we've defined `entityId` to need to be unique within the
realm, if the underlying impl happens be able to look up the entity without
checking `entityType` we should mention if it's safe for that impl to simpy
ignore this param.
Due to the nature of this, I would prefer to let it be `entityTypeHint` and
allow impls to ignore it, and distinguish the intended behavior from the strict
entityType in `readEntityByName`.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
