dimas-b commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2920945271
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java:
##########
@@ -112,29 +112,24 @@ public void addTopLevelName(String entityName,
PolarisEntityType entityType, boo
}
}
- /**
- * Adds a path that will be statically resolved with the primary Resolver
when resolveAll() is
- * called, and which contributes to the resolution status of whether all
paths have successfully
- * resolved.
- *
- * @param key the friendly lookup key for retrieving resolvedPaths after
resolveAll(); typically
- * might be a Namespace or TableIdentifier object.
- */
- public void addPath(ResolverPath path, Object key) {
+ /** Adds a statically resolved path using canonical registration semantics
only. */
+ public void addPath(ResolverPath path) {
primaryResolver.addPath(path);
- pathLookup.put(key, currentPathIndex);
+ // Preserve prior manifest lookup behavior: re-registering the same lookup
key overwrites the
+ // previous mapping (last-write-wins).
+ pathLookup.put(resolvedPathKey(path), currentPathIndex);
addedPaths.add(path);
++currentPathIndex;
}
- /**
- * Adds a path that is allowed to be dynamically resolved with a new
Resolver when
- * getPassthroughResolvedPath is called. These paths are also included in
the primary static
- * resolution set resolved during resolveAll().
- */
- public void addPassthroughPath(ResolverPath path, Object key) {
- addPath(path, key);
- passthroughPaths.put(key, path);
+ private static ResolvedPathKey resolvedPathKey(ResolverPath path) {
+ return ResolvedPathKey.of(path);
Review Comment:
nit: why not inline this trivial method redirect?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/ResolvedPathKey.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.persistence.resolver;
+
+import java.util.Arrays;
+import java.util.List;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.polaris.core.catalog.PolarisCatalogHelpers;
+import org.apache.polaris.core.entity.PolarisEntityType;
+
+/** Canonical lookup key for resolved paths in a {@link
PolarisResolutionManifest}. */
+public record ResolvedPathKey(List<String> entityNames, PolarisEntityType
entityType) {
+
+ public ResolvedPathKey {
+ entityNames = List.copyOf(entityNames);
+ }
+
+ public static ResolvedPathKey of(List<String> entityNames, PolarisEntityType
entityType) {
+ return new ResolvedPathKey(entityNames, entityType);
+ }
+
+ public static ResolvedPathKey of(ResolverPath path) {
+ return new ResolvedPathKey(path.entityNames(), path.lastEntityType());
Review Comment:
nit: `return path.key()`?
##########
runtime/service/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java:
##########
@@ -65,89 +60,34 @@ public PolarisResolvedPathWrapper
getResolvedReferenceCatalogEntity() {
}
@Override
- public PolarisResolvedPathWrapper getResolvedPath(Object key) {
+ public PolarisResolvedPathWrapper getResolvedPath(ResolvedPathKey key) {
PolarisResolutionManifest manifest = newResolutionManifest();
-
- if (key instanceof Namespace namespace) {
- manifest.addPath(
- new ResolverPath(Arrays.asList(namespace.levels()),
PolarisEntityType.NAMESPACE),
- namespace);
- manifest.resolveAll();
- return manifest.getResolvedPath(namespace);
- } else {
- throw new IllegalStateException(
- String.format(
- "Trying to getResolvedPath(key) for %s with class %s", key,
key.getClass()));
- }
+ manifest.addPath(new ResolverPath(key.entityNames(), key.entityType()));
Review Comment:
@dennishuo : is this critical?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java:
##########
@@ -342,25 +316,38 @@ public PolarisEntitySubType getLeafSubType(Object key) {
return resolved.get(resolved.size() - 1).getEntity().getSubType();
}
- /**
- * @param key the key associated with the path to retrieve that was
specified in addPath
- * @param prependRootContainer if true, also includes the rootContainer as
the first element of
- * the path; otherwise, the first element begins with the
referenceCatalog.
- * @return null if the path resolved for {@code key} isn't fully-resolved
when specified as
- * "optional"
- */
- public PolarisResolvedPathWrapper getResolvedPath(Object key, boolean
prependRootContainer) {
+ /** Resolves a registered path using a canonical lookup key. */
+ public PolarisResolvedPathWrapper getResolvedPath(
+ ResolvedPathKey key, boolean prependRootContainer) {
diagnostics.check(
pathLookup.containsKey(key),
"never_registered_key_for_resolved_path",
"key={} pathLookup={}",
key,
pathLookup);
+ return getResolvedPathByIndex(pathLookup.get(key), prependRootContainer);
+ }
+
+ public PolarisResolvedPathWrapper getResolvedPath(
+ ResolvedPathKey key, PolarisEntitySubType subType, boolean
prependRootContainer) {
+ PolarisResolvedPathWrapper resolvedPath = getResolvedPath(key,
prependRootContainer);
+ if (resolvedPath == null) {
+ return null;
+ }
+ if (!getIsPassthroughFacade()
+ && resolvedPath.getRawLeafEntity() != null
+ && subType != PolarisEntitySubType.ANY_SUBTYPE
+ && resolvedPath.getRawLeafEntity().getSubType() != subType) {
+ return null;
+ }
+ return resolvedPath;
+ }
+ private PolarisResolvedPathWrapper getResolvedPathByIndex(
Review Comment:
nit: there is only one trivial caller... do we need a separate method for
this? 🤔
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java:
##########
@@ -342,25 +316,38 @@ public PolarisEntitySubType getLeafSubType(Object key) {
return resolved.get(resolved.size() - 1).getEntity().getSubType();
}
- /**
- * @param key the key associated with the path to retrieve that was
specified in addPath
- * @param prependRootContainer if true, also includes the rootContainer as
the first element of
- * the path; otherwise, the first element begins with the
referenceCatalog.
- * @return null if the path resolved for {@code key} isn't fully-resolved
when specified as
- * "optional"
- */
- public PolarisResolvedPathWrapper getResolvedPath(Object key, boolean
prependRootContainer) {
+ /** Resolves a registered path using a canonical lookup key. */
+ public PolarisResolvedPathWrapper getResolvedPath(
+ ResolvedPathKey key, boolean prependRootContainer) {
diagnostics.check(
pathLookup.containsKey(key),
"never_registered_key_for_resolved_path",
"key={} pathLookup={}",
key,
pathLookup);
+ return getResolvedPathByIndex(pathLookup.get(key), prependRootContainer);
+ }
+
+ public PolarisResolvedPathWrapper getResolvedPath(
+ ResolvedPathKey key, PolarisEntitySubType subType, boolean
prependRootContainer) {
Review Comment:
nit: moving this to approx. old line 392 will probably simplify the diff.
##########
runtime/service/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java:
##########
@@ -65,89 +60,34 @@ public PolarisResolvedPathWrapper
getResolvedReferenceCatalogEntity() {
}
@Override
- public PolarisResolvedPathWrapper getResolvedPath(Object key) {
+ public PolarisResolvedPathWrapper getResolvedPath(ResolvedPathKey key) {
PolarisResolutionManifest manifest = newResolutionManifest();
-
- if (key instanceof Namespace namespace) {
- manifest.addPath(
- new ResolverPath(Arrays.asList(namespace.levels()),
PolarisEntityType.NAMESPACE),
- namespace);
- manifest.resolveAll();
- return manifest.getResolvedPath(namespace);
- } else {
- throw new IllegalStateException(
- String.format(
- "Trying to getResolvedPath(key) for %s with class %s", key,
key.getClass()));
- }
+ manifest.addPath(new ResolverPath(key.entityNames(), key.entityType()));
Review Comment:
Should we assert that the type inside `key` is still `NAMESPACE` for
backward compatibility?... although I personally think the new code is more
correct.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -303,10 +305,10 @@ private PolarisResolutionManifest
authorizeBasicCatalogRoleOperationOrThrow(
PolarisAuthorizableOperation op, String catalogName, String
catalogRoleName) {
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:
nit: `ResolverPathKey.ofCatalogRole(catalogRoleName).toPath()`?
--
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]
