Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
sungwy merged PR #3940: URL: https://github.com/apache/polaris/pull/3940 -- 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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2921665334
##
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 entityNames, PolarisEntityType
entityType) {
+
+ public ResolvedPathKey {
+entityNames = List.copyOf(entityNames);
+ }
+
+ public static ResolvedPathKey of(List entityNames, PolarisEntityType
entityType) {
Review Comment:
WFM :+1:
--
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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
sungwy commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2921645193
##
runtime/service/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java:
##
@@ -65,89 +62,42 @@ public PolarisResolvedPathWrapper
getResolvedReferenceCatalogEntity() {
}
@Override
- public PolarisResolvedPathWrapper getResolvedPath(Object key) {
+ public PolarisResolvedPathWrapper getResolvedPath(ResolvedPathKey key) {
+Preconditions.checkState(
+key.entityType() == PolarisEntityType.NAMESPACE,
+"Trying to getResolvedPath(key) for non-namespace key %s",
+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()));
+manifest.resolveAll();
+return manifest.getResolvedPath(key);
}
@Override
public PolarisResolvedPathWrapper getResolvedPath(
- Object key, PolarisEntityType entityType, PolarisEntitySubType subType) {
+ ResolvedPathKey key, PolarisEntitySubType subType) {
PolarisResolutionManifest manifest = newResolutionManifest();
-
-if (key instanceof TableIdentifier identifier) {
- manifest.addPath(
- new
ResolverPath(PolarisCatalogHelpers.tableIdentifierToList(identifier),
entityType),
- identifier);
- manifest.resolveAll();
- return manifest.getResolvedPath(identifier, entityType, subType);
-} else if (key instanceof PolicyIdentifier policyIdentifier) {
- manifest.addPath(
- new ResolverPath(
- PolarisCatalogHelpers.identifierToList(
- policyIdentifier.namespace(), policyIdentifier.name()),
- entityType),
- policyIdentifier);
- manifest.resolveAll();
- return manifest.getResolvedPath(policyIdentifier, entityType, subType);
-} else {
- throw new IllegalStateException(
- String.format(
- "Trying to getResolvedPath(key, subType) for %s with class %s
and type %s / %s",
- key, key.getClass(), entityType, subType));
-}
+manifest.addPath(new ResolverPath(key.entityNames(), key.entityType()));
+manifest.resolveAll();
+return manifest.getResolvedPath(key, subType);
}
@Override
- public PolarisResolvedPathWrapper getPassthroughResolvedPath(Object key) {
+ public PolarisResolvedPathWrapper getPassthroughResolvedPath(ResolvedPathKey
key) {
+Preconditions.checkState(
+key.entityType() == PolarisEntityType.NAMESPACE,
+"Trying to getPassthroughResolvedPath(key) for non-namespace key %s",
+key);
PolarisResolutionManifest manifest = newResolutionManifest();
-
-if (key instanceof Namespace namespace) {
- manifest.addPassthroughPath(
- new ResolverPath(Arrays.asList(namespace.levels()),
PolarisEntityType.NAMESPACE),
- namespace);
- return manifest.getPassthroughResolvedPath(namespace);
-} else {
- throw new IllegalStateException(
- String.format(
- "Trying to getResolvedPath(key) for %s with class %s", key,
key.getClass()));
-}
+manifest.addPassthroughPath(new ResolverPath(key.entityNames(),
key.entityType()));
+return manifest.getPassthroughResolvedPath(key);
}
@Override
public PolarisResolvedPathWrapper getPassthroughResolvedPath(
- Object key, PolarisEntityType entityType, PolarisEntitySubType subType) {
+ ResolvedPathKey key, PolarisEntitySubType subType) {
PolarisResolutionManifest manifest = newResolutionManifest();
-
-if (key instanceof TableIdentifier identifier) {
- manifest.addPassthroughPath(
- new
ResolverPath(PolarisCatalogHelpers.tableIdentifierToList(identifier),
entityType),
- identifier);
- return manifest.getPassthroughResolvedPath(identifier, entityType,
subType);
-} else if (key instanceof PolicyIdentifier policyIdentifier) {
- manifest.addPassthroughPath(
- new ResolverPath(
- PolarisCatalogHelpers.identifierToList(
- policyIdentifier.namespace(), policyIdentifier.name()),
- entityType),
- policyIdentifier);
- return manifest.getPassthroughResolvedPath(policyIdentifier, entityType,
subType);
-} else {
- throw new IllegalStateException(
Review Comment:
sure thing. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
sungwy commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2921641793
##
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 entityNames, PolarisEntityType
entityType) {
+
+ public ResolvedPathKey {
+entityNames = List.copyOf(entityNames);
+ }
+
+ public static ResolvedPathKey of(List entityNames, PolarisEntityType
entityType) {
Review Comment:
note to reviewer: this constructor isn't used right now outside of tests,
but I anticipate this will be used when we construct the key from within the
`PolarisAuthorizerImpl`
--
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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2921585753
##
runtime/service/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java:
##
@@ -65,89 +62,42 @@ public PolarisResolvedPathWrapper
getResolvedReferenceCatalogEntity() {
}
@Override
- public PolarisResolvedPathWrapper getResolvedPath(Object key) {
+ public PolarisResolvedPathWrapper getResolvedPath(ResolvedPathKey key) {
+Preconditions.checkState(
+key.entityType() == PolarisEntityType.NAMESPACE,
+"Trying to getResolvedPath(key) for non-namespace key %s",
+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()));
+manifest.resolveAll();
+return manifest.getResolvedPath(key);
}
@Override
public PolarisResolvedPathWrapper getResolvedPath(
- Object key, PolarisEntityType entityType, PolarisEntitySubType subType) {
+ ResolvedPathKey key, PolarisEntitySubType subType) {
PolarisResolutionManifest manifest = newResolutionManifest();
-
-if (key instanceof TableIdentifier identifier) {
- manifest.addPath(
- new
ResolverPath(PolarisCatalogHelpers.tableIdentifierToList(identifier),
entityType),
- identifier);
- manifest.resolveAll();
- return manifest.getResolvedPath(identifier, entityType, subType);
-} else if (key instanceof PolicyIdentifier policyIdentifier) {
- manifest.addPath(
- new ResolverPath(
- PolarisCatalogHelpers.identifierToList(
- policyIdentifier.namespace(), policyIdentifier.name()),
- entityType),
- policyIdentifier);
- manifest.resolveAll();
- return manifest.getResolvedPath(policyIdentifier, entityType, subType);
-} else {
- throw new IllegalStateException(
- String.format(
- "Trying to getResolvedPath(key, subType) for %s with class %s
and type %s / %s",
- key, key.getClass(), entityType, subType));
-}
+manifest.addPath(new ResolverPath(key.entityNames(), key.entityType()));
+manifest.resolveAll();
+return manifest.getResolvedPath(key, subType);
}
@Override
- public PolarisResolvedPathWrapper getPassthroughResolvedPath(Object key) {
+ public PolarisResolvedPathWrapper getPassthroughResolvedPath(ResolvedPathKey
key) {
+Preconditions.checkState(
+key.entityType() == PolarisEntityType.NAMESPACE,
+"Trying to getPassthroughResolvedPath(key) for non-namespace key %s",
+key);
PolarisResolutionManifest manifest = newResolutionManifest();
-
-if (key instanceof Namespace namespace) {
- manifest.addPassthroughPath(
- new ResolverPath(Arrays.asList(namespace.levels()),
PolarisEntityType.NAMESPACE),
- namespace);
- return manifest.getPassthroughResolvedPath(namespace);
-} else {
- throw new IllegalStateException(
- String.format(
- "Trying to getResolvedPath(key) for %s with class %s", key,
key.getClass()));
-}
+manifest.addPassthroughPath(new ResolverPath(key.entityNames(),
key.entityType()));
+return manifest.getPassthroughResolvedPath(key);
}
@Override
public PolarisResolvedPathWrapper getPassthroughResolvedPath(
- Object key, PolarisEntityType entityType, PolarisEntitySubType subType) {
+ ResolvedPathKey key, PolarisEntitySubType subType) {
PolarisResolutionManifest manifest = newResolutionManifest();
-
-if (key instanceof TableIdentifier identifier) {
- manifest.addPassthroughPath(
- new
ResolverPath(PolarisCatalogHelpers.tableIdentifierToList(identifier),
entityType),
- identifier);
- return manifest.getPassthroughResolvedPath(identifier, entityType,
subType);
-} else if (key instanceof PolicyIdentifier policyIdentifier) {
- manifest.addPassthroughPath(
- new ResolverPath(
- PolarisCatalogHelpers.identifierToList(
- policyIdentifier.namespace(), policyIdentifier.name()),
- entityType),
- policyIdentifier);
- return manifest.getPassthroughResolvedPath(policyIdentifier, entityType,
subType);
-} else {
- throw new IllegalStateException(
Review Comment:
restore this check too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHu
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2921583569
##
runtime/service/src/testFixtures/java/org/apache/polaris/service/catalog/PolarisPassthroughResolutionView.java:
##
@@ -65,89 +62,42 @@ public PolarisResolvedPathWrapper
getResolvedReferenceCatalogEntity() {
}
@Override
- public PolarisResolvedPathWrapper getResolvedPath(Object key) {
+ public PolarisResolvedPathWrapper getResolvedPath(ResolvedPathKey key) {
+Preconditions.checkState(
+key.entityType() == PolarisEntityType.NAMESPACE,
+"Trying to getResolvedPath(key) for non-namespace key %s",
+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()));
+manifest.resolveAll();
+return manifest.getResolvedPath(key);
}
@Override
public PolarisResolvedPathWrapper getResolvedPath(
- Object key, PolarisEntityType entityType, PolarisEntitySubType subType) {
+ ResolvedPathKey key, PolarisEntitySubType subType) {
PolarisResolutionManifest manifest = newResolutionManifest();
-
-if (key instanceof TableIdentifier identifier) {
- manifest.addPath(
- new
ResolverPath(PolarisCatalogHelpers.tableIdentifierToList(identifier),
entityType),
- identifier);
- manifest.resolveAll();
- return manifest.getResolvedPath(identifier, entityType, subType);
-} else if (key instanceof PolicyIdentifier policyIdentifier) {
- manifest.addPath(
- new ResolverPath(
- PolarisCatalogHelpers.identifierToList(
- policyIdentifier.namespace(), policyIdentifier.name()),
- entityType),
- policyIdentifier);
- manifest.resolveAll();
- return manifest.getResolvedPath(policyIdentifier, entityType, subType);
-} else {
- throw new IllegalStateException(
Review Comment:
now that the Namespace type check got restored, should we restore type
checks here as well?.
--
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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
sungwy commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2921539454
##
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:
I wasn’t fully sure why this test fixture previously restricted it to
Namespace only, since passthrough resolution can conceptually work for other
path types too. With `ResolvedPathKey`, we now have a uniform key, so I
generalized it.
That said, I agree we should preserve backward compatibility in this PR and
keep focus on the core key-standardization changes.
We can revisit test fixture related refactors separately.
--
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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
sungwy commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2921475678
##
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 entityNames, PolarisEntityType
entityType) {
+
+ public ResolvedPathKey {
+entityNames = List.copyOf(entityNames);
+ }
+
+ public static ResolvedPathKey of(List entityNames, PolarisEntityType
entityType) {
+return new ResolvedPathKey(entityNames, entityType);
+ }
+
+ public static ResolvedPathKey of(ResolverPath path) {
+return new ResolvedPathKey(path.entityNames(), path.lastEntityType());
Review Comment:
thank you for the nit. I didn't update this after I took the suggestion to
adopt composition
--
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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
sungwy commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2921472280
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/ResolverPath.java:
##
@@ -24,14 +24,14 @@
/**
* Simple class to represent a path within a catalog
*
- * @param entityNames name of the entities in that path. The parent of the
first named entity is the
- * path is the catalog
- * @param lastEntityType all entities in a path are namespaces except the last
one which can be a
- * table_like entity versus a namespace
+ * @param key canonical path key (entity names + terminal entity type)
* @param optional true if this path is optional, i.e. failing to fully
resolve it is not an error
*/
-public record ResolverPath(
-List entityNames, PolarisEntityType lastEntityType, boolean
optional) {
+public record ResolverPath(ResolvedPathKey key, boolean optional) {
+
+ public ResolverPath {
+key = new ResolvedPathKey(key.entityNames(), key.entityType());
Review Comment:
Good catch, I'll remove it since it's already immutable
--
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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
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 entityNames, PolarisEntityType
entityType) {
+
+ public ResolvedPathKey {
+entityNames = List.copyOf(entityNames);
+ }
+
+ public static ResolvedPathKey of(List 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(resol
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2920919748
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/ResolverPath.java:
##
@@ -24,14 +24,14 @@
/**
* Simple class to represent a path within a catalog
*
- * @param entityNames name of the entities in that path. The parent of the
first named entity is the
- * path is the catalog
- * @param lastEntityType all entities in a path are namespaces except the last
one which can be a
- * table_like entity versus a namespace
+ * @param key canonical path key (entity names + terminal entity type)
* @param optional true if this path is optional, i.e. failing to fully
resolve it is not an error
*/
-public record ResolverPath(
-List entityNames, PolarisEntityType lastEntityType, boolean
optional) {
+public record ResolverPath(ResolvedPathKey key, boolean optional) {
+
+ public ResolverPath {
+key = new ResolvedPathKey(key.entityNames(), key.entityType());
Review Comment:
`ResolvedPathKey` is already immutable, so why bother, indeed 🤔
--
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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
adutra commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2918639340
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/ResolverPath.java:
##
@@ -24,14 +24,14 @@
/**
* Simple class to represent a path within a catalog
*
- * @param entityNames name of the entities in that path. The parent of the
first named entity is the
- * path is the catalog
- * @param lastEntityType all entities in a path are namespaces except the last
one which can be a
- * table_like entity versus a namespace
+ * @param key canonical path key (entity names + terminal entity type)
* @param optional true if this path is optional, i.e. failing to fully
resolve it is not an error
*/
-public record ResolverPath(
-List entityNames, PolarisEntityType lastEntityType, boolean
optional) {
+public record ResolverPath(ResolvedPathKey key, boolean optional) {
+
+ public ResolverPath {
+key = new ResolvedPathKey(key.entityNames(), key.entityType());
Review Comment:
nit: do we need this defensive copy?
--
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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
sungwy commented on PR #3940: URL: https://github.com/apache/polaris/pull/3940#issuecomment-4039309774 Hi @dennishuo - just a bump to get your attention on this PR -- 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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on code in PR #3940: URL: https://github.com/apache/polaris/pull/3940#discussion_r2893115661 ## polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java: ## @@ -57,11 +57,11 @@ public class PolarisResolutionManifest implements PolarisResolutionManifestCatal private final Resolver primaryResolver; private final PolarisDiagnostics diagnostics; - private final Map pathLookup = new HashMap<>(); + private final Map pathLookup = new HashMap<>(); Review Comment: Sure. Totally fine to leave this out. -- 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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on PR #3940: URL: https://github.com/apache/polaris/pull/3940#issuecomment-4008727553 I second comments from @adutra above, but in general this PR LGTM :+1: Since it touches `polaris-core`, I hope @dennishuo could review too. -- 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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
sungwy commented on code in PR #3940: URL: https://github.com/apache/polaris/pull/3940#discussion_r2893087335 ## polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java: ## @@ -57,11 +57,11 @@ public class PolarisResolutionManifest implements PolarisResolutionManifestCatal private final Resolver primaryResolver; private final PolarisDiagnostics diagnostics; - private final Map pathLookup = new HashMap<>(); + private final Map pathLookup = new HashMap<>(); Review Comment: Hi @dimas-b - funny you mention this, because I was thinking just the same thing. But I fell short of proposing it because: 1. It wasn't absolutely necessary for the `PolarisAuthorizer` SPI refactoring needs 2. I wasn't really sure if there was some hidden sauce reason why we were doing the `Integer` based lookup... Since that's an underlying implementation detail, how about we leave that out of scope of this PR and revisit it after a careful, dedicated review? cc @dennishuo -- 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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on code in PR #3940: URL: https://github.com/apache/polaris/pull/3940#discussion_r2893009684 ## polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java: ## @@ -57,11 +57,11 @@ public class PolarisResolutionManifest implements PolarisResolutionManifestCatal private final Resolver primaryResolver; private final PolarisDiagnostics diagnostics; - private final Map pathLookup = new HashMap<>(); + private final Map pathLookup = new HashMap<>(); Review Comment: Actually, what I wrote above is not exactly what the old PR was doing... I guess I was dreaming a bit 😅 In any case, what I mean is this: ``` Handle h = resolutionManifest.addPath(...); // resolve PolarisResolvedPathWrapper r = h.get(); ``` WDYT? -- 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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on code in PR #3940: URL: https://github.com/apache/polaris/pull/3940#discussion_r2892983197 ## polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java: ## @@ -57,11 +57,11 @@ public class PolarisResolutionManifest implements PolarisResolutionManifestCatal private final Resolver primaryResolver; private final PolarisDiagnostics diagnostics; - private final Map pathLookup = new HashMap<>(); + private final Map pathLookup = new HashMap<>(); Review Comment: Ah, about why "handle" objects are beneficial: the callers can directly obtain the resolved entity from the handle object, no need to create `ResolvedPathKey` objects just for the sake of map lookup. Also, the handle is always directly related to the result on the caller side (local var.), which reduces risk of confusion when an unrelated key is used for lookup. -- 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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on code in PR #3940: URL: https://github.com/apache/polaris/pull/3940#discussion_r2892965291 ## polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java: ## @@ -57,11 +57,11 @@ public class PolarisResolutionManifest implements PolarisResolutionManifestCatal private final Resolver primaryResolver; private final PolarisDiagnostics diagnostics; - private final Map pathLookup = new HashMap<>(); + private final Map pathLookup = new HashMap<>(); Review Comment: I believe this map is generally used only to allow caller to retrieved resolved entities for some "seed" parameters. Some time ago I tried refactoring this code in #499 with the idea that callers would get back "handle" objects for "seed" parameter they put into the resolver flow. That PR did not merge, but I still wanted to mention it again. I wonder what you think about it. This is just an idea for consideration. No pressure 😉 I believe callers never need to use arbitrary lookup keys (at least they did not do that at the time of that old PR), they always obtain the resolved object for some kind of input parameter they already have, so the "handle" objects can usually be carried around as local variables. -- 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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
dimas-b commented on code in PR #3940: URL: https://github.com/apache/polaris/pull/3940#discussion_r2892965291 ## polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java: ## @@ -57,11 +57,11 @@ public class PolarisResolutionManifest implements PolarisResolutionManifestCatal private final Resolver primaryResolver; private final PolarisDiagnostics diagnostics; - private final Map pathLookup = new HashMap<>(); + private final Map pathLookup = new HashMap<>(); Review Comment: I believe this map is generally used only to allow caller to retrieved resolved entities for some "seed" parameters. Some time ago I tried refactoring this code in #499 with the idea that callers would get back "handle" objects for "seed" parameter the put into the resolver flow. That PR did not merge, but I still wanted to mention it again. I wonder what you think about it. This is just an idea for consideration. No pressure 😉 I believe callers never need to use arbitrary lookup keys (at least they did not do that at the time of that old PR), they always obtain the resolved object for some kind of input parameter they already have, so the "handle" objects can usually be carried around as local variables. -- 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]
Re: [PR] core: Standardize Path Lookup Key for `PolarisResolutionManifest.getResolvedPath` [polaris]
sungwy commented on code in PR #3940:
URL: https://github.com/apache/polaris/pull/3940#discussion_r2892112855
##
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 entityNames, PolarisEntityType entityType);
Review Comment:
yeah I really like this suggestion. let me adopt this
--
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]
