adutra commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2890924295
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -384,8 +387,7 @@ private PolarisResolutionManifest
authorizeGrantOnCatalogRoleToPrincipalRoleOper
String principalRoleName) {
PolarisResolutionManifest resolutionManifest =
newResolutionManifest(catalogName);
resolutionManifest.addPath(
- new ResolverPath(List.of(catalogRoleName),
PolarisEntityType.CATALOG_ROLE),
- catalogRoleName);
+ new ResolverPath(List.of(catalogRoleName),
PolarisEntityType.CATALOG_ROLE));
Review Comment:
I am observing the following similarity:
```java
private record ResolverPathKey(List<String> entityNames, PolarisEntityType
entityType) {}
public record ResolverPath (List<String> entityNames, PolarisEntityType
lastEntityType, boolean optional) {}
```
Wouldn't it be better to define these records by composition? E.g.:
```java
public record ResolverPathKey(List<String> entityNames, PolarisEntityType
entityType) {}
public record ResolverPath(ResolverPathKey key, boolean optional) {}
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifestCatalogView.java:
##########
@@ -39,13 +40,15 @@ public interface PolarisResolutionManifestCatalogView {
.orElse(null);
}
- PolarisResolvedPathWrapper getResolvedPath(Object key);
+ PolarisResolvedPathWrapper getResolvedPath(
+ List<String> entityNames, PolarisEntityType entityType);
Review Comment:
Would it be simpler to expose ResolverPathKey publicly and transform these
methods as follows:
```java
PolarisResolvedPathWrapper getResolvedPath(ResolvedPathKey key);
PolarisResolvedPathWrapper getResolvedPath(
ResolvedPathKey key, PolarisEntitySubType subType);
PolarisResolvedPathWrapper getPassthroughResolvedPath(
ResolvedPathKey key);
PolarisResolvedPathWrapper getPassthroughResolvedPath(
ResolvedPathKey key, PolarisEntitySubType subType);
```
Then `ResolvedPathKey` could expose some factory methods to quickly create
keys from different types of entities. E.g.
```java
resolvedEntityView.getResolvedPath(
PolarisCatalogHelpers.tableIdentifierToList(tableIdentifier),
PolarisEntityType.TABLE_LIKE,
PolarisEntitySubType.ICEBERG_TABLE);
```
Becomes:
```java
resolvedEntityView.getResolvedPath(
ResolvedPathKey.ofTable(tableIdentifier),
PolarisEntitySubType.ICEBERG_TABLE);
```
Just a suggestion :-)
--
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]
