HonahX commented on code in PR #1314:
URL: https://github.com/apache/polaris/pull/1314#discussion_r2029503484
##########
service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java:
##########
@@ -251,25 +248,224 @@ public Policy updatePolicy(
public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean
detachAll) {
// TODO: Implement detachAll when we support attach/detach policy
- PolarisResolvedPathWrapper resolvedEntities =
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var catalogPath = resolvedPolicyPath.getRawParentPath();
+ var policyEntity = resolvedPolicyPath.getRawLeafEntity();
+
+ return metaStoreManager
+ .dropEntityIfExists(
+ callContext.getPolarisCallContext(),
+ PolarisEntity.toCoreList(catalogPath),
+ policyEntity,
+ Map.of(),
+ false)
+ .isSuccess();
+ }
+
+ public boolean attachPolicy(
+ PolicyIdentifier policyIdentifier,
+ PolicyAttachmentTarget target,
+ Map<String, String> parameters) {
+
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var policyCatalogPath =
PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath());
+ var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity());
+
+ var resolvedTargetPath = getResolvedPathWrapper(target);
+ var targetCatalogPath =
PolarisEntity.toCoreList(resolvedTargetPath.getRawParentPath());
+ var targetEntity = resolvedTargetPath.getRawLeafEntity();
+
+ PolicyValidators.validateAttach(policyEntity, targetEntity);
+
+ var result =
+ metaStoreManager.attachPolicyToEntity(
+ callContext.getPolarisCallContext(),
+ targetCatalogPath,
+ targetEntity,
+ policyCatalogPath,
+ policyEntity,
+ parameters);
+
+ if (result.getReturnStatus() ==
POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS) {
+ var targetId = catalogEntity.getName();
+ if (target.getType() != CATALOG) {
+ targetId += "." + String.join(".", target.getPath());
+ }
+ throw new PolicyMappingAlreadyExistsException(
+ "The policy mapping of same type(%s) for %s already exists",
+ policyEntity.getPolicyType().getName(), targetId);
+ }
+
+ return result.isSuccess();
+ }
+
+ public boolean detachPolicy(PolicyIdentifier policyIdentifier,
PolicyAttachmentTarget target) {
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var policyCatalogPath =
PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath());
+ var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity());
+
+ var resolvedTargetPath = getResolvedPathWrapper(target);
+ var targetCatalogPath =
PolarisEntity.toCoreList(resolvedTargetPath.getRawParentPath());
+ var targetEntity = resolvedTargetPath.getRawLeafEntity();
+
+ return metaStoreManager
+ .detachPolicyFromEntity(
+ callContext.getPolarisCallContext(),
+ targetCatalogPath,
+ targetEntity,
+ policyCatalogPath,
+ policyEntity)
+ .isSuccess();
+ }
+
+ public List<Policy> getApplicablePolicies(
+ Namespace namespace, String targetName, PolicyType policyType) {
+ var targetFullPath = getFullPath(namespace, targetName);
+ return getEffectivePolicies(targetFullPath, policyType);
+ }
+
+ /**
+ * Returns the effective policies for a given hierarchical path and policy
type.
+ *
+ * <p>Potential Performance Improvements:
+ *
+ * <ul>
+ * <li>Range Query Optimization: Enhance the query mechanism to fetch
policies for all entities
+ * in a single range query, reducing the number of individual queries
against the mapping
+ * table.
+ * <li>Filtering on Inheritable: Improve the filtering process by applying
the inheritable
+ * condition at the data retrieval level, so that only the relevant
policies for non-leaf
+ * nodes are processed.
+ * <li>Caching: Implement caching for up-level policies to avoid redundant
calculations and
+ * lookups, especially for frequently accessed paths.
+ * </ul>
+ *
+ * @param path the list of entities representing the hierarchical path
+ * @param policyType the type of policy to filter on
+ * @return a list of effective policies, combining inherited policies from
upper levels and
+ * non-inheritable policies from the final entity
+ */
+ private List<Policy> getEffectivePolicies(List<PolarisEntity> path,
PolicyType policyType) {
+ if (path == null || path.isEmpty()) {
+ return List.of();
+ }
+
+ Map<String, Policy> inheritedPolicies = new LinkedHashMap<>();
+ // Final list of effective policies (inheritable + last-entity
non-inheritable)
+ List<Policy> finalPolicies = new ArrayList<>();
+
+ // Process all entities except the last one
+ for (int i = 0; i < path.size() - 1; i++) {
+ PolarisEntity entity = path.get(i);
+ List<Policy> currentPolicies = getPolicies(entity, policyType);
+
+ for (Policy policy : currentPolicies) {
+ // For non-last entities, we only carry forward inheritable policies
+ if (policy.getInheritable()) {
+ // Put in map; overwrites by policyType if encountered again
+ inheritedPolicies.put(policy.getPolicyType(), policy);
+ }
+ }
+ }
+
+ // Now handle the last entity's policies
+ List<Policy> lastPolicies = getPolicies(path.getLast(), policyType);
+
+ for (Policy policy : lastPolicies) {
+ if (policy.getInheritable()) {
+ // Overwrite anything by the same policyType in the inherited map
+ inheritedPolicies.put(policy.getPolicyType(), policy);
+ } else {
+ // Non-inheritable => goes directly to final list
+ finalPolicies.add(policy);
+ }
+ }
+
+ // Append all inherited policies at the end, preserving insertion order
+ finalPolicies.addAll(inheritedPolicies.values());
+
+ return finalPolicies;
+ }
+
+ private List<Policy> getPolicies(PolarisEntity target, PolicyType
policyType) {
+ LoadPolicyMappingsResult result;
+ if (policyType == null) {
+ result =
metaStoreManager.loadPoliciesOnEntity(callContext.getPolarisCallContext(),
target);
+ } else {
+ result =
+ metaStoreManager.loadPoliciesOnEntityByType(
+ callContext.getPolarisCallContext(), target, policyType);
+ }
+
+ return result.getEntities().stream().map(PolicyCatalog::toPolicy).toList();
Review Comment:
Do we want to convert `PolicyEntity` to `Policy` for every policy fetched in
this step? I think we can directly return a list of `PolicyEntity` here and
defer the conversion till we have a final list of effective `PolicyEntity`.
WDYT?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java:
##########
@@ -251,25 +248,224 @@ public Policy updatePolicy(
public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean
detachAll) {
// TODO: Implement detachAll when we support attach/detach policy
- PolarisResolvedPathWrapper resolvedEntities =
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var catalogPath = resolvedPolicyPath.getRawParentPath();
+ var policyEntity = resolvedPolicyPath.getRawLeafEntity();
+
+ return metaStoreManager
+ .dropEntityIfExists(
+ callContext.getPolarisCallContext(),
+ PolarisEntity.toCoreList(catalogPath),
+ policyEntity,
+ Map.of(),
+ false)
+ .isSuccess();
+ }
+
+ public boolean attachPolicy(
+ PolicyIdentifier policyIdentifier,
+ PolicyAttachmentTarget target,
+ Map<String, String> parameters) {
+
+ var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier);
+ var policyCatalogPath =
PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath());
+ var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity());
+
+ var resolvedTargetPath = getResolvedPathWrapper(target);
+ var targetCatalogPath =
PolarisEntity.toCoreList(resolvedTargetPath.getRawParentPath());
+ var targetEntity = resolvedTargetPath.getRawLeafEntity();
+
+ PolicyValidators.validateAttach(policyEntity, targetEntity);
+
+ var result =
+ metaStoreManager.attachPolicyToEntity(
+ callContext.getPolarisCallContext(),
+ targetCatalogPath,
+ targetEntity,
+ policyCatalogPath,
+ policyEntity,
+ parameters);
+
+ if (result.getReturnStatus() ==
POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS) {
+ var targetId = catalogEntity.getName();
+ if (target.getType() != CATALOG) {
+ targetId += "." + String.join(".", target.getPath());
+ }
+ throw new PolicyMappingAlreadyExistsException(
Review Comment:
I think we need to define a new exception that extends `PolarisException`
instead of using the one defined in the persistence. Otherwise, it won't be
mapped to 409 in http response
--
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
