jerryshao commented on code in PR #13478: URL: https://github.com/apache/gravitino/pull/13478#discussion_r4248023721
########## core/src/main/java/org/apache/gravitino/policy/rego/Comparison.java: ########## @@ -0,0 +1,119 @@ +/* + * 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.gravitino.policy.rego; + +import com.google.common.base.Preconditions; +import java.util.Objects; + +/** A binary comparison operation. */ +final class Comparison implements CanonicalExpression { Review Comment: [Question] The split also narrowed the whole AST from public to package-private - was that intended, or is it meant to be re-widened when the resolver lands? At 48cd060 the node types were nested inside the public `CanonicalExpression` interface, so `Comparison`, `Not`, `LogicalExpression`, `Column`, `SessionUser`, `GroupMembership`, `Literal`, `LiteralArray` and the three enums were all implicitly public, and their accessors were declared `public` (`public ComparisonOperator operator()`, `public CanonicalExpression left()`, ...). After 8d046c5 they are top-level classes with no modifier, and the accessors lost their `public` too. `CanonicalExpression` itself now exposes only `validate()` and `depth()`. The entry points did not move: `RestrictedRegoExpressionParserFacade.parse` is public, `RowFilter.lower()` (RestrictedRegoProgram.java:221), `RowFilter.branches()` (:191) and `FilterBranch.result()` / `condition()` (:121, :130) are all public and hand back `CanonicalExpression` values. Outside this package the only thing a caller can do with one is call `validate()` or `depth()` - it cannot tell a comparison from a negation, let alone read a column name. Nothing breaks today because `PolicyManager` is the only consumer and it discards the result, but design-docs/tag-based-read-restrictions-for-iceberg-rest.md:388-420 describes a later resolver that binds this tree to a schema and request context and emits an Iceberg wire predicate. If that resolver lives in another package it has to undo this. Either shape is defensible - keep the nodes public with a stable visitor, or make `lower()` and `branches()` package-private until the resolver exists so the public surface is just "parse and validate". The current mix is the one that will have to change. Worth saying either way, since the reply on the file-size thread described 8d046c5 as a structural refactor only; it is also an API-surface change. Verified by: diffed `git show 48cd060:core/src/main/java/org/apache/gravitino/policy/rego/CanonicalExpression.java` against the new files (nested types in an interface are implicitly public; the accessors were explicitly `public`), and grepped the tree for `policy.rego` references outside the package - only PolicyManager.java. -- 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]
