You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "bowenliang123 (via GitHub)" <gi...@apache.org> on 2023/06/02 01:53:39 UTC
[GitHub] [kyuubi] bowenliang123 opened a new pull request, #4916: [WIP] [AUTHZ]support ReplaceData
bowenliang123 opened a new pull request, #4916:
URL: https://github.com/apache/kyuubi/pull/4916
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
- Iceberg 1.3.0 use Spark's ReplaceData as Logical plan for "DELETE FROM"
- Requiring select privilege for input table, even it's the same with output table
- enable iceberg test for authz plugin
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [WIP] [AUTHZ] Support ReplaceData for Iceberg's DELETE FROM statement on Spark 3.4 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1356868949
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/IcebergCatalogPrivilegesBuilderSuite.scala:
##########
@@ -57,7 +58,15 @@ class IcebergCatalogPrivilegesBuilderSuite extends V2CommandsPrivilegesSuite {
val plan = sql(s"DELETE FROM $catalogTable WHERE key = 1 ").queryExecution.analyzed
val (inputs, outputs, operationType) = PrivilegesBuilder.build(plan, spark)
assert(operationType === QUERY)
- assert(inputs.isEmpty)
+ if (isSparkV34OrGreater) {
+ assert(inputs.size === 1)
+ val po = inputs.head
+ assertEqualsIgnoreCase(namespace)(po.dbname)
+ assertEqualsIgnoreCase(catalogTableShort)(po.objectName)
+ assertContains(po.columns, "key", "value")
Review Comment:
SGTM
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on a diff in pull request #4916: [WIP] [AUTHZ]support ReplaceData
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1213840756
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilderSuite.scala:
##########
@@ -74,8 +74,9 @@ abstract class PrivilegesBuilderSuite extends AnyFunSuite
po: PrivilegeObject,
expectedOwner: String = defaultTableOwner): Unit = {
if (catalogImpl == "hive" && po.privilegeObjectType === PrivilegeObjectType.TABLE_OR_VIEW) {
- assert(po.owner.isDefined)
- assert(po.owner.get === expectedOwner)
+ // TODO: fixme
+ // assert(po.owner.isDefined)
+ // assert(po.owner.get === expectedOwner)
Review Comment:
Temporarily skipping checking table owner. And reported to https://github.com/apache/kyuubi/issues/4917.
cc @zhouyifan279
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1357644821
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -106,12 +113,10 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
withSingleCallEnabled {
val e2 = intercept[AccessControlException](
doAs(
- someone,
+ bob,
sql(mergeIntoSql)))
assert(e2.getMessage.contains(s"does not have" +
- s" [select] privilege" +
- s" on [$namespace1/$table1/id,$namespace1/table1/name,$namespace1/$table1/city]," +
- s" [update] privilege on [$namespace1/$outputTable1]"))
+ s" [update] privilege on [$bobNamespace/$bobSelectTable]"))
}
Review Comment:
`withSingleCallEnabled` is for listing all the unsatisfied privileges in the exception. We still need a negative case for `someone` who is anonymous and has no permission on the source and target table at all.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1765486473
Two minor issues left in this PR,
1. the CI test 'Kyuubi Server On Kubernetes Integration Test' have never succeeded, but it seems irrelevant to the changes in Authz plugin.
2. this PR may not be able to merge to branch-1.8, as it merged from the master branch of 1.9 several times.
Is that Spark 3.4 and 3.5 support part of the goal of 1.8.x ? cc @pan3793
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1360015204
##########
pom.xml:
##########
@@ -2239,7 +2239,7 @@
<delta.version>2.4.0</delta.version>
<spark.version>3.4.1</spark.version>
<spark.binary.version>3.4</spark.binary.version>
- <maven.plugin.scalatest.exclude.tags>org.scalatest.tags.Slow,org.apache.kyuubi.tags.IcebergTest</maven.plugin.scalatest.exclude.tags>
Review Comment:
is there any context to remove this tag ?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1764067974
This PR is ready for merging. With this PR, Authz plugin tests can be enabled for Spark 3.4 and 3.5. cc @yaooqinn @pan3793
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1765675422
@bowenliang123 it's intended to make Spark 3.4 fully supported in 1.8, which of the previous patches are required?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [WIP] [AUTHZ] Support ReplaceData for Iceberg's DELETE FROM statement on Spark 3.4 [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1356756619
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -47,6 +47,8 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
val namespace1 = icebergNamespace
val table1 = "table1"
val outputTable1 = "outputTable1"
+ val bobNamespace= "default_bob"
+ val bobSelectTable = "table_select_bob_1"
Review Comment:
Query permission is required when using ReplaceData to execute `DELETE/UPDATE/MERGEINTO` in spark 3.5. So I created a new `default_bob.table_select_bob_1` table. For bob, it only has select permissions but no update permissions.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [WIP] [AUTHZ] Support ReplaceData for Iceberg's DELETE FROM statement on Spark 3.4 [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1356360061
##########
extensions/spark/kyuubi-spark-authz/src/main/resources/table_command_spec.json:
##########
@@ -470,6 +470,27 @@
} ],
"opType" : "ALTERTABLE_REPLACECOLS",
"queryDescs" : [ ]
+}, {
+ "classname" : "org.apache.spark.sql.catalyst.plans.logical.ReplaceData",
+ "tableDescs" : [ {
+ "fieldName" : "originalTable",
Review Comment:
`ReplaceData.table` is replaced by `RowLevelOperationTable` without properties, so `originalTable`is more suitable.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1765721652
Thanks for the help. Most of the credit for fixing the compatibility goes to @wForget . You made this happen by direct pushing the complete solution after I tried to abandon it for over 1 month.
Also many thanks to @pan3793 for cherry-picking this PR and backporting to 1.8.0.
Now with this PR merged to branch-1.8 (1.8.0) and master(1.9.0), the Authz plugin is tested on Spark 3.4 and Spark 3.5.
Thank you all.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1763742593
LGTM.
As discussed above (https://github.com/apache/kyuubi/pull/4916#discussion_r1357665415), Iceberg on Spark 3.5 would have a subquery for selecting the table first, which triggers the exception for the SELECT privileges check solely first. So we prepared separate assertion exception messages for either Spark 3.5 or lower version.
If we agree with this change, please approve this PR, as this is my PR and your review is required. @wForget
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1765680085
I think this PR is the last required one for Spark 3.4 support in Authz.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [WIP] [AUTHZ] Support ReplaceData for Iceberg's DELETE FROM statement on Spark 3.4 [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1356365811
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/IcebergCatalogPrivilegesBuilderSuite.scala:
##########
@@ -74,7 +83,15 @@ class IcebergCatalogPrivilegesBuilderSuite extends V2CommandsPrivilegesSuite {
val plan = sql(s"UPDATE $catalogTable SET value = 'b' WHERE key = 1 ").queryExecution.analyzed
val (inputs, outputs, operationType) = PrivilegesBuilder.build(plan, spark)
assert(operationType === QUERY)
- assert(inputs.isEmpty)
+ if (isSparkV35OrGreater) {
Review Comment:
This is also implemented using ReplaceData in Spark 3.5.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1360050769
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -106,12 +113,10 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
withSingleCallEnabled {
val e2 = intercept[AccessControlException](
doAs(
- someone,
+ bob,
sql(mergeIntoSql)))
assert(e2.getMessage.contains(s"does not have" +
- s" [select] privilege" +
- s" on [$namespace1/$table1/id,$namespace1/table1/name,$namespace1/$table1/city]," +
- s" [update] privilege on [$namespace1/$outputTable1]"))
+ s" [update] privilege on [$bobNamespace/$bobSelectTable]"))
}
Review Comment:
Let's bring back the single call ut for `someone` with no permissions to ensure end-to-end results.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1763671251
Seems that if `someone` without any permission, it does not check the `update` privilege on the target output table.
```
- [KYUUBI #3515] MERGE INTO *** FAILED ***
"Permission denied: user [someone] does not have [select] privilege on [iceberg_ns/table1/id,iceberg_ns/table1/name,iceberg_ns/table1/city]" did not contain "[select] privilege on [iceberg_ns/table1/id,iceberg_ns/table1/name,iceberg_ns/table1/city], [update] privilege on [default_bob/table_select_bob_1]" (IcebergCatalogRangerSparkExtensionSuite.scala:114)
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [WIP] [AUTHZ] Support ReplaceData for Iceberg's DELETE FROM statement on Spark 3.4 [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1356364211
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/IcebergCatalogPrivilegesBuilderSuite.scala:
##########
@@ -57,7 +58,15 @@ class IcebergCatalogPrivilegesBuilderSuite extends V2CommandsPrivilegesSuite {
val plan = sql(s"DELETE FROM $catalogTable WHERE key = 1 ").queryExecution.analyzed
val (inputs, outputs, operationType) = PrivilegesBuilder.build(plan, spark)
assert(operationType === QUERY)
- assert(inputs.isEmpty)
+ if (isSparkV34OrGreater) {
+ assert(inputs.size === 1)
+ val po = inputs.head
+ assertEqualsIgnoreCase(namespace)(po.dbname)
+ assertEqualsIgnoreCase(catalogTableShort)(po.objectName)
+ assertContains(po.columns, "key", "value")
Review Comment:
`po.columns` may also have additional metadataColumns.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [WIP] [AUTHZ] Support ReplaceData for Iceberg's DELETE FROM statement on Spark 3.4 and 3.5 [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1759816345
@bowenliang123 I think this pr is ready to be reviewed and the check actions executed successfully. cc @yaooqinn @ulysses-you
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on pull request #4916: [WIP] [AUTHZ] Support ReplaceData for Iceberg's DELETE FROM statement
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1573191212
Curiously to see the unexpected required fields in input for ReplaceData plan with Iceberg table , including hidden columns `_file` and `_pos`, and also the untouched column`value`.
With "DELETE FROM iceberg_table WHERE key = 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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] codecov-commenter commented on pull request #4916: [WIP] [AUTHZ] Support ReplaceData for Iceberg's DELETE FROM statement
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1573250236
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/4916?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#4916](https://app.codecov.io/gh/apache/kyuubi/pull/4916?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (70a7e50) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/fc8460b89118f3845e31dfe3fef61e43d3ca4437?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (fc8460b) will **not change** coverage.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #4916 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 557 557
Lines 30694 30695 +1
Branches 3995 3996 +1
======================================
- Misses 30694 30695 +1
```
| [Impacted Files](https://app.codecov.io/gh/apache/kyuubi/pull/4916?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...he/kyuubi/plugin/spark/authz/util/AuthZUtils.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4916?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3V0aWwvQXV0aFpVdGlscy5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#issuecomment-1765708058
Cherry-picked to branch-1.8 via https://github.com/apache/kyuubi/commit/3f9c6761ebb0fd4428ae0fca9b2d0c4ad2bdcb9f
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1360026780
##########
pom.xml:
##########
@@ -2239,7 +2239,7 @@
<delta.version>2.4.0</delta.version>
<spark.version>3.4.1</spark.version>
<spark.binary.version>3.4</spark.binary.version>
- <maven.plugin.scalatest.exclude.tags>org.scalatest.tags.Slow,org.apache.kyuubi.tags.IcebergTest</maven.plugin.scalatest.exclude.tags>
Review Comment:
> is there any context to remove this tag ?
The test cases containing the `IcebergTest` tag have executed successfully, I think we can remove it.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 closed pull request #4916: [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5
URL: https://github.com/apache/kyuubi/pull/4916
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [WIP] [AUTHZ] Support ReplaceData for Iceberg's DELETE FROM statement on Spark 3.4 [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1356367918
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/IcebergCatalogPrivilegesBuilderSuite.scala:
##########
@@ -98,8 +115,20 @@ class IcebergCatalogPrivilegesBuilderSuite extends V2CommandsPrivilegesSuite {
s"WHEN NOT MATCHED THEN INSERT *").queryExecution.analyzed
val (inputs, outputs, operationType) = PrivilegesBuilder.build(plan, spark)
assert(operationType === QUERY)
- assert(inputs.size === 1)
- val po0 = inputs.head
+ if (isSparkV35OrGreater) {
Review Comment:
ditto
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1357665415
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -106,12 +113,10 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
withSingleCallEnabled {
val e2 = intercept[AccessControlException](
doAs(
- someone,
+ bob,
sql(mergeIntoSql)))
assert(e2.getMessage.contains(s"does not have" +
- s" [select] privilege" +
- s" on [$namespace1/$table1/id,$namespace1/table1/name,$namespace1/$table1/city]," +
- s" [update] privilege on [$namespace1/$outputTable1]"))
+ s" [update] privilege on [$bobNamespace/$bobSelectTable]"))
}
Review Comment:
There is subQuery in the `mergeIntoSql` execution plan in Spark 3.5. When applying the `OptimizeSubqueries` rule, it returns no select permission and no update. If we want to verify `withSingleCallEnabled` I think we need to use other statements, like: `insert into ... select ...`
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [AUTHZ] Support ReplaceData and compatible Spark 3.4 and 3.5 [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #4916:
URL: https://github.com/apache/kyuubi/pull/4916#discussion_r1357670188
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -106,12 +113,10 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
withSingleCallEnabled {
val e2 = intercept[AccessControlException](
doAs(
- someone,
+ bob,
sql(mergeIntoSql)))
assert(e2.getMessage.contains(s"does not have" +
- s" [select] privilege" +
- s" on [$namespace1/$table1/id,$namespace1/table1/name,$namespace1/$table1/city]," +
- s" [update] privilege on [$namespace1/$outputTable1]"))
+ s" [update] privilege on [$bobNamespace/$bobSelectTable]"))
}
Review Comment:
> If we want to verify withSingleCallEnabled I think we need to use other statements, like: insert into ... select ...
It is covered in:
```
"[KYUUBI #3371] support throws all disallowed privileges in exception"
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org