Skip to content

Commit c1685c6

Browse files
AngersZhuuuuyaooqinn
authored andcommitted
[KYUUBI #5780][AUTHZ] Treating PermanentViewMarker as LeafNode make code simple and got correct privilege object
# 🔍 Description ## Issue References 🔗 This pull request fixes #5780 ## Describe Your Solution 🔧 Currently, we convert persist view to PermanentViewMaker, but after optimizer, it changed its child, making it hard to do column prune and get the right column privilege object of persist view. In this pr, we change PVM as a LeafNode, then we can directly treat it as a `HiveRelation` since we won't change its internal plan to make code simpler. But we need to re-optimize the child plan after do privilege check. ## Types of changes 🔖 - [x] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ For sql such as below table and view ``` CREATE TABLE IF NOT EXISTS $db1.$table1(id int, scope int); CREATE TABLE IF NOT EXISTS $db1.$table2( id int, name string, age int, scope int) .stripMargin); CREATE VIEW $db1.$view1 AS WITH temp AS ( SELECT max(scope) max_scope FROM $db1.$table1) SELECT id, name, max(scope) as max_scope, sum(age) sum_age FROM $db1.$table2 WHERE scope in (SELECT max_scope FROM temp) GROUP BY id, name ``` When we execute query on `$db1.$view1` ``` SELECT id as new_id, name, max_scope FROM $db1.$view1 ``` It will first execute the subquery in the query, then got a un-correct column privilege #### Behavior With This Pull Request 🎉 After this change, since PVM is a LeafNode, we won't execute the subquery under PVM, and we directly got the correct column privilege. #### Related Unit Tests Existed UT --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [x] Pull request title is okay. - [x] No license issues. - [x] Milestone correctly set? - [x] Test coverage is ok - [x] Assignees are selected. - [x] Minimum number of approvals - [x] No changes are requested **Be nice. Be informative.** Closes #5781 from AngersZhuuuu/KYUUBI-5780. Closes #5780 3c18bb7 [Angerszhuuuu] Merge branch 'master' into KYUUBI-5780 64f7947 [Angerszhuuuu] update b4f6fc0 [Angerszhuuuu] Merge branch 'master' into KYUUBI-5780 fbc989a [Angerszhuuuu] Update Authorization.scala 2113cf5 [Angerszhuuuu] Update RuleApplyPermanentViewMarker.scala 5dbe232 [Angerszhuuuu] Update WithInternalChildren.scala 04a40c3 [Angerszhuuuu] update 57bf5ba [Angerszhuuuu] update 738d506 [Angerszhuuuu] Update RuleApplyPermanentViewMarker.scala bade427 [Angerszhuuuu] [KYUUBI #5780][AUTHZ] Kyuubi tread PVM ass LeafNode to make logic more simple Authored-by: Angerszhuuuu <angers.zhu@gmail.com> Signed-off-by: Kent Yao <yao@apache.org>
1 parent eb9e88b commit c1685c6

File tree

11 files changed

+31
-48
lines changed

11 files changed

+31
-48
lines changed

extensions/spark/kyuubi-spark-authz/src/main/resources/database_command_spec.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,4 +215,4 @@
215215
} ],
216216
"opType" : "SWITCHDATABASE",
217217
"uriDescs" : [ ]
218-
} ]
218+
} ]

extensions/spark/kyuubi-spark-authz/src/main/resources/function_command_spec.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,4 +111,4 @@
111111
"comment" : ""
112112
} ],
113113
"opType" : "RELOADFUNCTION"
114-
} ]
114+
} ]

extensions/spark/kyuubi-spark-authz/src/main/resources/scan_command_spec.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,4 +111,4 @@
111111
"comment" : ""
112112
} ],
113113
"uriDescs" : [ ]
114-
} ]
114+
} ]

extensions/spark/kyuubi-spark-authz/src/main/resources/table_command_spec.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2528,4 +2528,4 @@
25282528
"isInput" : false,
25292529
"comment" : "Delta"
25302530
} ]
2531-
} ]
2531+
} ]

extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import org.slf4j.LoggerFactory
2828
import org.apache.kyuubi.plugin.spark.authz.OperationType.OperationType
2929
import org.apache.kyuubi.plugin.spark.authz.PrivilegeObjectActionType._
3030
import org.apache.kyuubi.plugin.spark.authz.rule.Authorization._
31-
import org.apache.kyuubi.plugin.spark.authz.rule.permanentview.PermanentViewMarker
3231
import org.apache.kyuubi.plugin.spark.authz.rule.rowfilter._
3332
import org.apache.kyuubi.plugin.spark.authz.serde._
3433
import org.apache.kyuubi.plugin.spark.authz.util.AuthZUtils._
@@ -68,13 +67,7 @@ object PrivilegesBuilder {
6867

6968
def mergeProjection(table: Table, plan: LogicalPlan): Unit = {
7069
if (projectionList.isEmpty) {
71-
plan match {
72-
case pvm: PermanentViewMarker
73-
if pvm.isSubqueryExpressionPlaceHolder || pvm.output.isEmpty =>
74-
privilegeObjects += PrivilegeObject(table, pvm.outputColNames)
75-
case _ =>
76-
privilegeObjects += PrivilegeObject(table, plan.output.map(_.name))
77-
}
70+
privilegeObjects += PrivilegeObject(table, plan.output.map(_.name))
7871
} else {
7972
val cols = (projectionList ++ conditionList).flatMap(collectLeaves)
8073
.filter(plan.outputSet.contains).map(_.name).distinct

extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtension.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ class RangerSparkExtension extends (SparkSessionExtensions => Unit) {
5353
v1.injectResolutionRule(RuleApplyDataMaskingStage1)
5454
v1.injectOptimizerRule(_ => new RuleEliminateMarker())
5555
v1.injectOptimizerRule(new RuleAuthorization(_))
56-
v1.injectOptimizerRule(_ => new RuleEliminatePermanentViewMarker())
56+
v1.injectOptimizerRule(new RuleEliminatePermanentViewMarker(_))
5757
v1.injectOptimizerRule(_ => new RuleEliminateTypeOf())
5858
v1.injectPlannerStrategy(new FilterDataSourceV2Strategy(_))
5959
}

extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/rule/Authorization.scala

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,12 @@
1818
package org.apache.kyuubi.plugin.spark.authz.rule
1919

2020
import org.apache.spark.sql.SparkSession
21-
import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Subquery}
21+
import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, View}
2222
import org.apache.spark.sql.catalyst.rules.Rule
2323
import org.apache.spark.sql.catalyst.trees.TreeNodeTag
2424
import org.apache.spark.sql.execution.SQLExecution.EXECUTION_ID_KEY
2525

2626
import org.apache.kyuubi.plugin.spark.authz.rule.Authorization._
27-
import org.apache.kyuubi.plugin.spark.authz.rule.permanentview.PermanentViewMarker
2827
import org.apache.kyuubi.plugin.spark.authz.util.ReservedKeys._
2928

3029
abstract class Authorization(spark: SparkSession) extends Rule[LogicalPlan] {
@@ -51,21 +50,18 @@ object Authorization {
5150
}
5251
}
5352

54-
protected def markAuthChecked(plan: LogicalPlan): LogicalPlan = {
53+
def markAuthChecked(plan: LogicalPlan): LogicalPlan = {
5554
plan.setTagValue(KYUUBI_AUTHZ_TAG, ())
5655
plan transformDown {
57-
case pvm: PermanentViewMarker =>
58-
markAllNodesAuthChecked(pvm)
59-
case subquery: Subquery =>
60-
markAllNodesAuthChecked(subquery)
56+
// TODO: Add this line Support for spark3.1, we can remove this
57+
// after spark 3.2 since https://issues.apache.org/jira/browse/SPARK-34269
58+
case view: View =>
59+
markAllNodesAuthChecked(view.child)
6160
}
6261
}
6362

6463
protected def isAuthChecked(plan: LogicalPlan): Boolean = {
65-
plan match {
66-
case subquery: Subquery => isAuthChecked(subquery.child)
67-
case p => p.getTagValue(KYUUBI_AUTHZ_TAG).nonEmpty
68-
}
64+
plan.getTagValue(KYUUBI_AUTHZ_TAG).nonEmpty
6965
}
7066

7167
def setExplainCommandExecutionId(sparkSession: SparkSession): Unit = {

extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/rule/RuleEliminatePermanentViewMarker.scala

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
package org.apache.kyuubi.plugin.spark.authz.rule
1919

20+
import org.apache.spark.sql.SparkSession
2021
import org.apache.spark.sql.catalyst.expressions.SubqueryExpression
2122
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
2223
import org.apache.spark.sql.catalyst.rules.Rule
@@ -26,12 +27,21 @@ import org.apache.kyuubi.plugin.spark.authz.rule.permanentview.PermanentViewMark
2627
/**
2728
* Transforming up [[PermanentViewMarker]]
2829
*/
29-
class RuleEliminatePermanentViewMarker extends Rule[LogicalPlan] {
30+
class RuleEliminatePermanentViewMarker(sparkSession: SparkSession) extends Rule[LogicalPlan] {
3031
override def apply(plan: LogicalPlan): LogicalPlan = {
31-
plan.transformUp {
32-
case pvm: PermanentViewMarker => pvm.child.transformAllExpressions {
32+
var matched = false
33+
val eliminatedPVM = plan.transformUp {
34+
case pvm: PermanentViewMarker =>
35+
matched = true
36+
pvm.child.transformAllExpressions {
3337
case s: SubqueryExpression => s.withNewPlan(apply(s.plan))
3438
}
3539
}
40+
if (matched) {
41+
Authorization.markAuthChecked(eliminatedPVM)
42+
sparkSession.sessionState.optimizer.execute(eliminatedPVM)
43+
} else {
44+
eliminatedPVM
45+
}
3646
}
3747
}

extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/rule/permanentview/PermanentViewMarker.scala

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,20 +19,9 @@ package org.apache.kyuubi.plugin.spark.authz.rule.permanentview
1919

2020
import org.apache.spark.sql.catalyst.catalog.CatalogTable
2121
import org.apache.spark.sql.catalyst.expressions.Attribute
22-
import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, UnaryNode}
22+
import org.apache.spark.sql.catalyst.plans.logical.{LeafNode, LogicalPlan}
2323

24-
import org.apache.kyuubi.plugin.spark.authz.util.WithInternalChild
25-
26-
case class PermanentViewMarker(
27-
child: LogicalPlan,
28-
catalogTable: CatalogTable,
29-
outputColNames: Seq[String],
30-
isSubqueryExpressionPlaceHolder: Boolean = false) extends UnaryNode
31-
with WithInternalChild {
24+
case class PermanentViewMarker(child: LogicalPlan, catalogTable: CatalogTable) extends LeafNode {
3225

3326
override def output: Seq[Attribute] = child.output
34-
35-
override def withNewChildInternal(newChild: LogicalPlan): LogicalPlan =
36-
copy(child = newChild)
37-
3827
}

extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/rule/permanentview/RuleApplyPermanentViewMarker.scala

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,9 @@ class RuleApplyPermanentViewMarker extends Rule[LogicalPlan] {
3838
case permanentView: View if hasResolvedPermanentView(permanentView) =>
3939
val resolved = permanentView.transformAllExpressions {
4040
case subquery: SubqueryExpression =>
41-
subquery.withNewPlan(plan =
42-
PermanentViewMarker(
43-
subquery.plan,
44-
permanentView.desc,
45-
permanentView.output.map(_.name),
46-
true))
41+
subquery.withNewPlan(plan = PermanentViewMarker(subquery.plan, permanentView.desc))
4742
}
48-
PermanentViewMarker(resolved, resolved.desc, resolved.output.map(_.name))
43+
PermanentViewMarker(resolved, permanentView.desc)
4944
case other => apply(other)
5045
}
5146
}

0 commit comments

Comments
 (0)