You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/09/19 15:20:30 UTC
[GitHub] [incubator-kyuubi] bowenliang123 opened a new pull request, #3520: [KYUUBI #3515] [Authz] support UpdateIcebergTable/MergeIntoIcebergTable/DeleteFromIcebergTable from Iceberg
bowenliang123 opened a new pull request, #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520
<!--
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/incubator-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.
-->
to close #3515.
By replacing mapChildren in `RuleApplyRowFilterAndDataMasking`to skip head of children query as insterted by iceberg in `IcebergSparkSessionExtensions` .
### _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
- [ ] [Run test](https://kyuubi.apache.org/docs/latest/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
[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#issuecomment-1256278967
cc @yaooqinn , please have a check if some time available.
--
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] [incubator-kyuubi] codecov-commenter commented on pull request #3520: [KYUUBI #3515] [Authz] skip apply Row-filter to output table of rewrited Iceberg command (UpdateIcebergTable / MergeIntoIcebergTable / DeleteFromIcebergTable )
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#issuecomment-1251873384
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3520](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a1f33bc) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/f8892f6cb6a66046c5cb61cf5c93c5e410134938?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f8892f6) will **increase** coverage by `0.01%`.
> The diff coverage is `87.93%`.
```diff
@@ Coverage Diff @@
## master #3520 +/- ##
============================================
+ Coverage 51.72% 51.73% +0.01%
Complexity 13 13
============================================
Files 482 483 +1
Lines 26933 26990 +57
Branches 3760 3768 +8
============================================
+ Hits 13930 13964 +34
- Misses 11652 11664 +12
- Partials 1351 1362 +11
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...uthz/ranger/RuleApplyRowFilterAndDataMasking.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3Jhbmdlci9SdWxlQXBwbHlSb3dGaWx0ZXJBbmREYXRhTWFza2luZy5zY2FsYQ==) | `86.27% <81.25%> (-2.62%)` | :arrow_down: |
| [...he/kyuubi/plugin/spark/authz/IcebergCommands.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L0ljZWJlcmdDb21tYW5kcy5zY2FsYQ==) | `90.00% <90.00%> (ø)` | |
| [.../kyuubi/plugin/spark/authz/PrivilegesBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L1ByaXZpbGVnZXNCdWlsZGVyLnNjYWxh) | `69.51% <100.00%> (+0.18%)` | :arrow_up: |
| [.../kyuubi/server/mysql/constant/MySQLErrorCode.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbXlzcWwvY29uc3RhbnQvTXlTUUxFcnJvckNvZGUuc2NhbGE=) | `13.84% <0.00%> (-6.16%)` | :arrow_down: |
| [...ache/kyuubi/server/mysql/MySQLCommandHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbXlzcWwvTXlTUUxDb21tYW5kSGFuZGxlci5zY2FsYQ==) | `75.00% <0.00%> (-4.55%)` | :arrow_down: |
| [...apache/kyuubi/engine/JpsApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvSnBzQXBwbGljYXRpb25PcGVyYXRpb24uc2NhbGE=) | `77.41% <0.00%> (-3.23%)` | :arrow_down: |
| [...ache/kyuubi/server/mysql/MySQLGenericPackets.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbXlzcWwvTXlTUUxHZW5lcmljUGFja2V0cy5zY2FsYQ==) | `76.59% <0.00%> (-2.13%)` | :arrow_down: |
| [...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvY21kL2xvZy9Mb2dCYXRjaENvbW1hbmQuc2NhbGE=) | `80.00% <0.00%> (-2.00%)` | :arrow_down: |
| [...pache/kyuubi/engine/YarnApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvWWFybkFwcGxpY2F0aW9uT3BlcmF0aW9uLnNjYWxh) | `62.96% <0.00%> (-1.86%)` | :arrow_down: |
| [...mon/src/main/scala/org/apache/kyuubi/Logging.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9Mb2dnaW5nLnNjYWxh) | `52.63% <0.00%> (-1.32%)` | :arrow_down: |
| ... and [4 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3520/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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=The+Apache+Software+Foundation)
--
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] [incubator-kyuubi] ulysses-you commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r990995288
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
how about this ?
```scala
private def mapPlanChildren(plan: LogicalPlan)(f: LogicalPlan => LogicalPlan): LogicalPlan = {
val newChildren = plan match {
case _ if IcebergCommands.accept(plan.nodeName) =>
val skipped = IcebergCommands.skipMappedChildren(plan)
skipped ++ (plan.children diff skipped).map(f)
case _ =>
plan.children.map(f)
}
plan.withNewChildren(newChildren)
}
```
--
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] [incubator-kyuubi] yaooqinn commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r991745320
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/SparkSessionProvider.scala:
##########
@@ -46,6 +48,7 @@ trait SparkSessionProvider {
.config(
"spark.sql.warehouse.dir",
Files.createTempDirectory("spark-warehouse").toString)
+ .config("spark.sql.extensions", sqlExtensions)
Review Comment:
why do we need this?
--
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] [incubator-kyuubi] bowenliang123 commented on pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#issuecomment-1273990476
Thanks very much for the guidance and patience from all of you.
And now with this PR with Iceberg catalog/extension integrated, it also helps us to test more features and checks on Spark V2 catalogs. The previously introduced V2JDBCcatalog is not fully functioned with all V2 catalog features.
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981340738
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
Yes, and for `MergeIntoIcebergTable`command even has two tables unrealted to child querys in the head as `targetTable :: sourceTable :: rewritePlan.get :: Nil`. We may consider a more isolated methods to handle children query for different privatly rewritten commands from Iceberg or other projects in the future. But so for right now, it's good enough to fix this problem in one general handling way.
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r991746681
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/SparkSessionProvider.scala:
##########
@@ -46,6 +48,7 @@ trait SparkSessionProvider {
.config(
"spark.sql.warehouse.dir",
Files.createTempDirectory("spark-warehouse").toString)
+ .config("spark.sql.extensions", sqlExtensions)
Review Comment:
It's for adding Iceberg sql extension for testing rewritten commands in iceberg catalog.
"org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions"
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981305824
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
SkippedMapChildren is in the heads of children of the rewritten commands by iceberg, the order is not changed.
as UpdateTable in Iceberg,
```
override def children: Seq[LogicalPlan] = if (rewritePlan.isDefined) {
table :: rewritePlan.get :: Nil
} else {
table :: Nil
}
```
--
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] [incubator-kyuubi] ulysses-you commented on pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#issuecomment-1273987894
late lgtm
--
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] [incubator-kyuubi] ulysses-you commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r990872871
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
I think it is still a general way. The difference is minor :
```scala
val icebergCommands = Set("MergeIntoIcebergTable", "UpdateTable", ...)
case if icebergCommands.contains(plan.nodeName) => // do your code
case _ => original code
```
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981318507
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/IcebergCommands.scala:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.kyuubi.plugin.spark.authz
+
+import scala.collection.mutable.ArrayBuffer
+
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+
+import org.apache.kyuubi.plugin.spark.authz.OperationType._
+import org.apache.kyuubi.plugin.spark.authz.PrivilegeObjectActionType.PrivilegeObjectActionType
+import org.apache.kyuubi.plugin.spark.authz.util.AuthZUtils._
+import org.apache.kyuubi.plugin.spark.authz.v2Commands.CommandType.CommandType
+
+/**
+ * Building privilege objects
+ * for Iceberg commands rewritten by extension
+ */
+object IcebergCommands extends Enumeration {
+
+ import scala.language.implicitConversions
+
+ implicit def valueToCmdPrivilegeBuilder(x: Value): CmdPrivilegeBuilder =
+ x.asInstanceOf[CmdPrivilegeBuilder]
+
+ /**
+ * check whether commandName is implemented with supported privilege builders
+ * and pass the requirement checks (e.g. Spark version)
+ *
+ * @param commandName name of command
+ * @return true if so, false else
+ */
+ def accept(commandName: String): Boolean = {
+ try {
+ val command = IcebergCommands.withName(commandName)
+
+ // check spark version requirements
+ def passSparkVersionCheck: Boolean =
+ (command.mostVer.isEmpty || isSparkVersionAtMost(command.mostVer.get)) &&
+ (command.leastVer.isEmpty || isSparkVersionAtLeast(command.leastVer.get))
+
+ passSparkVersionCheck
Review Comment:
ok, and moved to Authz for generalization. reused in v2Command privilege building.
--
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] [incubator-kyuubi] pan3793 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981278328
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -44,6 +44,15 @@ private[authz] object AuthZUtils {
}
}
+ def getFieldValOption[T](o: Any, name: String): Option[T] = {
+ try {
+ val value = getFieldVal[T](o, name)
+ Some(value)
+ } catch {
+ case _: Exception => None
+ }
+ }
Review Comment:
```suggestion
def getFieldValOpt[T](o: Any, name: String): Option[T] = Try(getFieldVal[T](o, name)).toOption
```
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
This may change the order of children, does it matter? cc @ulysses-you
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -0,0 +1,145 @@
+/*
+ * 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.kyuubi.plugin.spark.authz.ranger
+
+// scalastyle:off
+import java.nio.file.Files
+
+import org.apache.kyuubi.plugin.spark.authz.AccessControlException
+
+/**
+ * Tests for RangerSparkExtensionSuite
+ * on Iceberg catalog with DataSource V2 API.
+ */
+class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite {
+ override protected val catalogImpl: String = "hive"
+ override protected val sqlExtensions: String =
+ if (isSparkV32OrGreater)
Review Comment:
Iceberg supports Spark 3.1 as well, but starting from 3.2 is good either
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/IcebergCommands.scala:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.kyuubi.plugin.spark.authz
+
+import scala.collection.mutable.ArrayBuffer
+
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
+
+import org.apache.kyuubi.plugin.spark.authz.OperationType._
+import org.apache.kyuubi.plugin.spark.authz.PrivilegeObjectActionType.PrivilegeObjectActionType
+import org.apache.kyuubi.plugin.spark.authz.util.AuthZUtils._
+import org.apache.kyuubi.plugin.spark.authz.v2Commands.CommandType.CommandType
+
+/**
+ * Building privilege objects
+ * for Iceberg commands rewritten by extension
+ */
+object IcebergCommands extends Enumeration {
+
+ import scala.language.implicitConversions
+
+ implicit def valueToCmdPrivilegeBuilder(x: Value): CmdPrivilegeBuilder =
+ x.asInstanceOf[CmdPrivilegeBuilder]
+
+ /**
+ * check whether commandName is implemented with supported privilege builders
+ * and pass the requirement checks (e.g. Spark version)
+ *
+ * @param commandName name of command
+ * @return true if so, false else
+ */
+ def accept(commandName: String): Boolean = {
+ try {
+ val command = IcebergCommands.withName(commandName)
+
+ // check spark version requirements
+ def passSparkVersionCheck: Boolean =
+ (command.mostVer.isEmpty || isSparkVersionAtMost(command.mostVer.get)) &&
+ (command.leastVer.isEmpty || isSparkVersionAtLeast(command.leastVer.get))
+
+ passSparkVersionCheck
Review Comment:
```suggestion
command.mostVer.forall(isSparkVersionAtMost) &&
command.leastVer.forall(isSparkVersionAtMost)
```
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981979110
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
Yes, in my first version of implementation, this skip step applies only to Iceberg Commands. And @yaooqinn suggested a general way applied to all. Do you have any better idea about this?
And after all, these changes are guaranteed with all existing ut passed.
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981307844
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -44,6 +44,15 @@ private[authz] object AuthZUtils {
}
}
+ def getFieldValOption[T](o: Any, name: String): Option[T] = {
+ try {
+ val value = getFieldVal[T](o, name)
+ Some(value)
+ } catch {
+ case _: Exception => None
+ }
+ }
Review Comment:
good suggestion, accepted.
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981331675
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -0,0 +1,145 @@
+/*
+ * 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.kyuubi.plugin.spark.authz.ranger
+
+// scalastyle:off
+import java.nio.file.Files
+
+import org.apache.kyuubi.plugin.spark.authz.AccessControlException
+
+/**
+ * Tests for RangerSparkExtensionSuite
+ * on Iceberg catalog with DataSource V2 API.
+ */
+class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite {
+ override protected val catalogImpl: String = "hive"
+ override protected val sqlExtensions: String =
+ if (isSparkV32OrGreater)
Review Comment:
Yes, iceberg catalog introduced in this pr is intended to support rewritten commands as DeleteFromIcebergTable,UpdateIcebergTable, and MergeIntoIcebergTable , which are all introduced since spark 3.2+.
For other v2 ut, they should be covered in `V2JdbcTableCatalogRangerSparkExtensionSuite` for spark 3.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] [incubator-kyuubi] pan3793 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981327708
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
quite tricky ...
--
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] [incubator-kyuubi] ulysses-you commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981928000
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
it's really dangerous, we can only do this for the command UpdateTable or MergeIntoIcebergTable. And keep the original code for safe
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r981979110
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
Yes, in my first implementation, this skip step applies only for Iceberg Commands. And @yaooqinn suggested a general way applied to all. Do you have any better idea for this ?
And after all, these changes are guaranteed with all existing ut passed.
--
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] [incubator-kyuubi] yaooqinn commented on pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#issuecomment-1273986568
merged to master
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r990870457
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
Is there any further suggestion on this point or this PR so that we could push this PR forward?
It has been blocked for almost half a month since the last passed commit.
@yaooqinn @pan3793 @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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r991017809
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
Wow thanks, cool! It looks much more elegant.
Accepted and pushed the commit with this change just now.
--
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] [incubator-kyuubi] yaooqinn closed pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
yaooqinn closed pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
URL: https://github.com/apache/incubator-kyuubi/pull/3520
--
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] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3520: [KYUUBI #3515] [Authz] support checking rewritten Iceberg commands and skip apply Row-filter to output tables
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#discussion_r990921568
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleApplyRowFilterAndDataMasking.scala:
##########
@@ -30,30 +30,51 @@ import org.apache.kyuubi.plugin.spark.authz.util.RowFilterAndDataMaskingMarker
class RuleApplyRowFilterAndDataMasking(spark: SparkSession) extends Rule[LogicalPlan] {
+ private def mapPlanChildren(plan: LogicalPlan, f: LogicalPlan => LogicalPlan): LogicalPlan = {
+ val planChildren = plan.children
+ if (planChildren.isEmpty) {
+ plan
+ } else {
+ // skip effected table within plan's children
+ val skippedMapChildren: Seq[LogicalPlan] = Seq(
+ getFieldValOption[LogicalPlan](plan, "table"),
+ getFieldValOption[LogicalPlan](plan, "targetTable"),
+ getFieldValOption[LogicalPlan](plan, "sourceTable"))
+ .collect {
+ case t if t.isDefined => t.get
+ } intersect planChildren
+
+ val mappedChildren = (planChildren diff skippedMapChildren).map(f)
+ plan.withNewChildren(skippedMapChildren ++ mappedChildren)
Review Comment:
OK, I have accepted your suggestion and pushed the changes to this,
```
val skippedMapChildren = plan match {
case _ if IcebergCommands.accept(plan.nodeName) =>
IcebergCommands.skipMappedChildren(plan)
case _ =>
Seq.empty
}
val mappedChildren = (planChildren diff skippedMapChildren).map(f)
plan.withNewChildren(skippedMapChildren ++ mappedChildren)
```
My intention is to 1. only skip children plans for Iceberg commands , 2. prevent map children twice in different condition branch and remain the capability to skip children logical plans for any other commands.
What do you think?
--
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] [incubator-kyuubi] bowenliang123 commented on pull request #3520: [KYUUBI #3515] [Authz] support UpdateIcebergTable/MergeIntoIcebergTable/DeleteFromIcebergTable from Iceberg
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3520:
URL: https://github.com/apache/incubator-kyuubi/pull/3520#issuecomment-1251173756
1. Introduced a `IcebergTableCatalogRangerSparkExtensionSuite` with iceberg's `IcebergSparkSessionExtensions` for unit test.
2. Replacing mapChildren in RuleApplyRowFilterAndDataMaskingto skip head of children query as insterted by iceberg in IcebergSparkSessionExtensions .
Currently it passed the `UPDATE TABLE` in `IcebergTableCatalogRangerSparkExtensionSuite`
--
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