gh-yzou commented on code in PR #1147:
URL: https://github.com/apache/polaris/pull/1147#discussion_r1988120676
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -373,7 +373,8 @@ public ResolverStatus resolveAll() {
// validate input
diagnostics.check(
- entityType != PolarisEntityType.NAMESPACE && entityType !=
PolarisEntityType.TABLE_LIKE,
Review Comment:
do you know why this function does not handle TABLE_LIKE, the comment is not
clear about why, I assume the GENEIC_TABLE should work similar as TABLE_LIKE.
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrivilege.java:
##########
@@ -41,22 +41,22 @@ public enum PolarisPrivilege {
TABLE_CREATE(6, PolarisEntityType.NAMESPACE),
VIEW_CREATE(7, PolarisEntityType.NAMESPACE),
NAMESPACE_DROP(8, PolarisEntityType.NAMESPACE),
- TABLE_DROP(9, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.TABLE),
- VIEW_DROP(10, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.VIEW),
+ TABLE_DROP(9, PolarisEntityType.ICEBERG_TABLE_LIKE,
PolarisEntitySubType.TABLE),
Review Comment:
One thing we mentioned in the doc is that we will reuse the current table
privilege, so that people don't need to grant both TABLE_DROP and
GENERIC_TABLE_DROP, which could be quite confusing to users.
With this setup, i don't think it is easy to extend the definition, it seems
more nature to me that we can define the privilege with set of subtype, but one
main type. Maybe what we can do is to have the base TableLikeEntity with just
identifier, and then we can extend it to have IcebergTableLikeEntity and
GenericTableEntity, in the future if we want to have a DeltaTable or general
VIEW entity, we can all extend on top of the TableLikeEntity.
Then here we can extend the PolarisPrivilege to take a list of SubType to
reuse the Table Privileges.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogPrefixParser.java:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.catalog;
+package org.apache.polaris.service.catalog.iceberg;
Review Comment:
The methods in this class seems not really needed for generic tables, I
think it is fine if we want to still keep it as Iceberg specific for now.
However, I think let's get an independent pr for all renaming and relocation of
iceberg classes separately for reivew.
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityType.java:
##########
@@ -32,9 +32,10 @@ public enum PolarisEntityType {
CATALOG_ROLE(5, CATALOG, true, false),
NAMESPACE(6, CATALOG, false, true),
// generic table is either a view or a real table
- TABLE_LIKE(7, NAMESPACE, false, false),
+ ICEBERG_TABLE_LIKE(7, NAMESPACE, false, false),
Review Comment:
Is EntityType and EntitySubType something also persisted? if yes, we
probably will need to add some backward compatibility support for this, so that
the previous TableLikeEntity can be extracted to the correct application level
entity, right?
--
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]
