REVOKE GRANT OPTION FOR should revoke only GRANT OPTION#10094
REVOKE GRANT OPTION FOR should revoke only GRANT OPTION#10094Praveen2112 merged 5 commits intotrinodb:masterfrom
Conversation
kokosing
left a comment
kokosing
left a comment
There was a problem hiding this comment.
Please add more test cases. When you revoke and then grant again for example.
See https://www.postgresql.org/docs/14/sql-revoke.html which follows:
If the privilege or the grant option held by the first user is being revoked and dependent privileges exist, those dependent privileges are also revoked if CASCADE is specified
Please add tests around this case to concrete the Hive behavior.
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestGrantRevoke.java
Outdated
Show resolved
Hide resolved
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestGrantRevoke.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java
Outdated
Show resolved
Hide resolved
|
Do we plan a fallback for Hive metastores older than 0.14.0? |
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestGrantRevoke.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I suggest also testing the inverse - adding the GRANT OPTION back to the privilege once it was removed.
There was a problem hiding this comment.
This is not supported by HiveMetastore directly - will be addressing them in a different PR.
Praveen2112
force-pushed
the
praveen/hive_sql_std_access
branch
2 times, most recently
from
dd668aa to
47c2109
Compare
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestGrantRevoke.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
What should happen if it is not successful? Show we retry or fail? Do we check that? I don't see any check for this in io.trino.plugin.hive.metastore.thrift.ThriftHiveMetastore#revokeTablePrivileges
There was a problem hiding this comment.
Why do we need this change at all? Is it only for symmetry with revokePrivileges?
There was a problem hiding this comment.
Why do we need this change at all
Client#grant_privileges is deprecated so we move to the new API. And yet it would be symmetric with revokePrivileges.
There was a problem hiding this comment.
Show we retry or fail.
We retry in case of any failures.
But here we just leave it as it is. Like we do for dropPartitions
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestGrantRevoke.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Why do we need this change at all? Is it only for symmetry with revokePrivileges?
Praveen2112
force-pushed
the
praveen/hive_sql_std_access
branch
from
47c2109 to
540cd6b
Compare
|
@kokosing AC |
This allows ThriftMetastore APIs to know the actual context of GRANT/REVOKE statement like with or without GRANT OPTION.
Praveen2112
force-pushed
the
praveen/hive_sql_std_access
branch
from
540cd6b to
5b5fa80
Compare
4 participants
No description provided.