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/10/08 15:22:09 UTC
[GitHub] [incubator-kyuubi] packyan opened a new pull request, #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
packyan opened a new pull request, #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593
<!--
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 #3592
### _How was this patch tested?_
- [x] 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.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 a diff in pull request #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r996314892
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -83,6 +84,27 @@ object PrivilegesBuilder {
spark.sessionState.catalog.isTempView(parts)
}
+ private def isPersistentFunction(
+ functionIdent: FunctionIdentifier,
+ spark: SparkSession): Boolean = {
+ val (db, funcName) = functionIdent.database match {
+ case Some(db) =>
+ (db, functionIdent.funcName)
+ case _ =>
+ Try {
+ spark.sessionState.catalog.lookupFunctionInfo(functionIdent)
Review Comment:
Is this `lookupFunctionInfo` necessary for function identifier with null dbs,
And in what case it will be will out with funInfo?
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider persistent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r998235233
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -385,8 +414,18 @@ object PrivilegesBuilder {
inputObjs += databasePrivileges(quote(database))
case "DescribeFunctionCommand" =>
- val func = getPlanField[FunctionIdentifier]("functionName")
- inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ val (db: Option[String], funName: String) =
Review Comment:
Yes, alright.
--
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] packyan commented on a diff in pull request #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
packyan commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r996402135
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -385,8 +414,18 @@ object PrivilegesBuilder {
inputObjs += databasePrivileges(quote(database))
case "DescribeFunctionCommand" =>
- val func = getPlanField[FunctionIdentifier]("functionName")
- inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ val (db: Option[String], funName: String) =
Review Comment:
Getting the FunctionIdentifier via the `functionName` field might not be that common, so maybe we can generalize it as a method the next time we need to get the FunctionIdentifier?
--
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] packyan commented on a diff in pull request #3593: [KYUUBI #3592] Spark SQL authz only consider persistent functions
Posted by GitBox <gi...@apache.org>.
packyan commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r997049147
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -83,6 +84,27 @@ object PrivilegesBuilder {
spark.sessionState.catalog.isTempView(parts)
}
+ private def isPersistentFunction(
+ functionIdent: FunctionIdentifier,
+ spark: SparkSession): Boolean = {
+ val (db, funcName) = functionIdent.database match {
+ case Some(db) =>
+ (db, functionIdent.funcName)
+ case _ =>
+ Try {
+ spark.sessionState.catalog.lookupFunctionInfo(functionIdent)
Review Comment:
sounds reasonable, when db is missing, we use current db to check wheather it's persistent.
`spark.sessionState.catalog.isPersistentFunction(FunctionIdentifier(funcName, database.or(spark.catalog.currentDatabase)))`
--
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] packyan commented on a diff in pull request #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
packyan commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r996299922
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -386,7 +390,10 @@ object PrivilegesBuilder {
case "DescribeFunctionCommand" =>
val func = getPlanField[FunctionIdentifier]("functionName")
- inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ val isTemp = spark.sessionState.catalog.isTemporaryFunction(func)
Review Comment:
isTempFunction is not suitable for here, I have changed it to check the functioon is persistent or not, as built-in function is also not temporay, so generalizing a `isPersistentFunction` method is more reasonable？
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r994708358
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -386,7 +390,10 @@ object PrivilegesBuilder {
case "DescribeFunctionCommand" =>
val func = getPlanField[FunctionIdentifier]("functionName")
- inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ val isTemp = spark.sessionState.catalog.isTemporaryFunction(func)
Review Comment:
Consider generalizing it in a method `isTempFunction` for checking the temp function in this class, as the `isTempView` method.
--
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 closed pull request #3593: [KYUUBI #3592] Spark SQL authz only consider persistent functions
Posted by GitBox <gi...@apache.org>.
pan3793 closed pull request #3593: [KYUUBI #3592] Spark SQL authz only consider persistent functions
URL: https://github.com/apache/incubator-kyuubi/pull/3593
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r995842243
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -386,7 +393,10 @@ object PrivilegesBuilder {
case "DescribeFunctionCommand" =>
val func = getPlanField[FunctionIdentifier]("functionName")
Review Comment:
```suggestion
val (db: Option[String], funName: String) =
if (isSparkVersionAtLeast("3.3")) {
val info = getPlanField[ExpressionInfo]("info")
if (info.getDb != null) info.getDb + "." + info.getName else info.getName
(Some(info.getDb), info.getName)
} else {
val funcIdent = getPlanField[FunctionIdentifier]("functionName")
(funcIdent.database, funcIdent.funcName)
}```
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r994708358
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -386,7 +390,10 @@ object PrivilegesBuilder {
case "DescribeFunctionCommand" =>
val func = getPlanField[FunctionIdentifier]("functionName")
- inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ val isTemp = spark.sessionState.catalog.isTemporaryFunction(func)
Review Comment:
Consider generalizing it in a method for checking the temp function in this class, as the `isTempView` method.
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider persistent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r998242398
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -83,6 +84,27 @@ object PrivilegesBuilder {
spark.sessionState.catalog.isTempView(parts)
}
+ private def isPersistentFunction(
+ functionIdent: FunctionIdentifier,
+ spark: SparkSession): Boolean = {
+ val (db, funcName) = functionIdent.database match {
+ case Some(db) =>
+ (db, functionIdent.funcName)
+ case _ =>
+ Try {
+ spark.sessionState.catalog.lookupFunctionInfo(functionIdent)
Review Comment:
Let's just skip filling the missing db name.
As in `spark.sessionState.catalog.isPersistentFunction`, it will be auto filled with current db name.
```
def isPersistentFunction(name: FunctionIdentifier): Boolean = {
val db = formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
...
```
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r996315466
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -385,8 +414,18 @@ object PrivilegesBuilder {
inputObjs += databasePrivileges(quote(database))
case "DescribeFunctionCommand" =>
- val func = getPlanField[FunctionIdentifier]("functionName")
- inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ val (db: Option[String], funName: String) =
Review Comment:
shall we directly get a `FunctionIdentifier` here and generalize it to AuthUtils like `getFunctionIdentifier` if possible?
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r995846277
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -347,8 +347,15 @@ object PrivilegesBuilder {
buildQuery(getQuery, inputObjs)
case "CreateFunctionCommand" |
- "DropFunctionCommand" |
- "RefreshFunctionCommand" =>
+ "DropFunctionCommand" =>
+ val db = getPlanField[Option[String]]("databaseName")
+ val functionName = getPlanField[String]("functionName")
+ val isTemp = getPlanField[Boolean]("isTemp")
+ if (!isTemp) {
+ outputObjs += functionPrivileges(db.orNull, functionName)
+ }
Review Comment:
```suggestion
val isTemp = getPlanField[Boolean]("isTemp")
if (!isTemp) {
val db = getPlanField[Option[String]]("databaseName")
val functionName = getPlanField[String]("functionName")
outputObjs += functionPrivileges(db.orNull, functionName)
}
```
We could check if it's temp function first, before fetching names for db and function.
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r995842243
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -386,7 +393,10 @@ object PrivilegesBuilder {
case "DescribeFunctionCommand" =>
val func = getPlanField[FunctionIdentifier]("functionName")
Review Comment:
```suggestion
val (db: Option[String], funName: String) =
if (isSparkVersionAtLeast("3.3")) {
val info = getPlanField[ExpressionInfo]("info")
(Some(info.getDb), info.getName)
} else {
val funcIdent = getPlanField[FunctionIdentifier]("functionName")
(funcIdent.database, funcIdent.funcName)
}```
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r995842243
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -386,7 +393,10 @@ object PrivilegesBuilder {
case "DescribeFunctionCommand" =>
val func = getPlanField[FunctionIdentifier]("functionName")
Review Comment:
```suggestion
val (db: Option[String], funName: String) =
if (isSparkVersionAtLeast("3.3")) {
val info = getPlanField[ExpressionInfo]("info")
(Some(info.getDb), info.getName)
} else {
val funcIdent = getPlanField[FunctionIdentifier]("functionName")
(funcIdent.database, funcIdent.funcName)
}
```
To support changes in Spark 3.3
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r996301737
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -386,7 +390,10 @@ object PrivilegesBuilder {
case "DescribeFunctionCommand" =>
val func = getPlanField[FunctionIdentifier]("functionName")
- inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ val isTemp = spark.sessionState.catalog.isTemporaryFunction(func)
Review Comment:
Yes, good to have 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
[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3593: [KYUUBI #3592] Spark SQL authz only consider persistent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r998242398
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -83,6 +84,27 @@ object PrivilegesBuilder {
spark.sessionState.catalog.isTempView(parts)
}
+ private def isPersistentFunction(
+ functionIdent: FunctionIdentifier,
+ spark: SparkSession): Boolean = {
+ val (db, funcName) = functionIdent.database match {
+ case Some(db) =>
+ (db, functionIdent.funcName)
+ case _ =>
+ Try {
+ spark.sessionState.catalog.lookupFunctionInfo(functionIdent)
Review Comment:
Let's just skip filling the missing db name.
As in `spark.sessionState.catalog.isPersistentFunction`, it will be auto filled with current db name.
Code of `isPersistentFunction` in Spark:
```
def isPersistentFunction(name: FunctionIdentifier): Boolean = {
val db = formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
...
```
--
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 pull request #3593: [KYUUBI #3592] Spark SQL authz only consider persistent functions
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#issuecomment-1285456290
thanks @packyan and @bowenliang123, merging 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] codecov-commenter commented on pull request #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#issuecomment-1272351977
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593?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 [#3593](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c7005b3) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/612a82e00c997971f3ca24f191b5fd1597434099?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (612a82e) will **decrease** coverage by `0.04%`.
> The diff coverage is `71.42%`.
```diff
@@ Coverage Diff @@
## master #3593 +/- ##
============================================
- Coverage 51.81% 51.77% -0.05%
Complexity 13 13
============================================
Files 483 483
Lines 26998 27003 +5
Branches 3769 3771 +2
============================================
- Hits 13990 13980 -10
- Misses 11651 11661 +10
- Partials 1357 1362 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/plugin/spark/authz/PrivilegesBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593/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) | `70.09% <71.42%> (+0.76%)` | :arrow_up: |
| [.../kyuubi/server/mysql/constant/MySQLErrorCode.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593/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/3593/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/3593/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/3593/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: |
| [...pache/kyuubi/engine/YarnApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593/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: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvRW5naW5lUmVmLnNjYWxh) | `72.26% <0.00%> (-0.85%)` | :arrow_down: |
| [...yuubi/server/metadata/jdbc/JDBCMetadataStore.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbWV0YWRhdGEvamRiYy9KREJDTWV0YWRhdGFTdG9yZS5zY2FsYQ==) | `89.27% <0.00%> (-0.70%)` | :arrow_down: |
| [...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9ldGNkL0V0Y2REaXNjb3ZlcnlDbGllbnQuc2NhbGE=) | `66.25% <0.00%> (-0.63%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `80.74% <0.00%> (-0.63%)` | :arrow_down: |
| ... and [6 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3593/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] packyan commented on a diff in pull request #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
packyan commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r996401819
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -83,6 +84,27 @@ object PrivilegesBuilder {
spark.sessionState.catalog.isTempView(parts)
}
+ private def isPersistentFunction(
+ functionIdent: FunctionIdentifier,
+ spark: SparkSession): Boolean = {
+ val (db, funcName) = functionIdent.database match {
+ case Some(db) =>
+ (db, functionIdent.funcName)
+ case _ =>
+ Try {
+ spark.sessionState.catalog.lookupFunctionInfo(functionIdent)
Review Comment:
> Is this `lookupFunctionInfo` necessary for function identifier with null dbs, And in what case it will be will out with funInfo?
In query 'DESCRIBE FUNCTION fun1', DescribeFunctionCommand's functionName field is `FunctionIdentifier(funName, None)`, even the fun1 is a persistent function, unless you use the full name: 'DESCRIBE FUNCTION db.fun1'.
So, it's do necessary in #isPersistentFunction method.
--
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] packyan commented on a diff in pull request #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
packyan commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r996402135
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -385,8 +414,18 @@ object PrivilegesBuilder {
inputObjs += databasePrivileges(quote(database))
case "DescribeFunctionCommand" =>
- val func = getPlanField[FunctionIdentifier]("functionName")
- inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ val (db: Option[String], funName: String) =
Review Comment:
Getting the FunctionIdentifier via the `functionName` field might not be that frequent, so maybe we can generalize it as a method the next time we need to get the FunctionIdentifier?
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r996446324
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -83,6 +84,27 @@ object PrivilegesBuilder {
spark.sessionState.catalog.isTempView(parts)
}
+ private def isPersistentFunction(
+ functionIdent: FunctionIdentifier,
+ spark: SparkSession): Boolean = {
+ val (db, funcName) = functionIdent.database match {
+ case Some(db) =>
+ (db, functionIdent.funcName)
+ case _ =>
+ Try {
+ spark.sessionState.catalog.lookupFunctionInfo(functionIdent)
Review Comment:
`lookupFunctionInfo` is a heavy action after all.
And I think we have resolved functions and check the privileges only for here.
So even if db name is missing, let's just fill it with current db name , just like in `spark.sessionState.catalog.lookupFunctionInfo`.
Shall try some ut without `lookupFunctionInfo` to verify its necessity？
--
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 #3593: [KYUUBI #3592] Spark SQL authz only consider permanent functions
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3593:
URL: https://github.com/apache/incubator-kyuubi/pull/3593#discussion_r995841560
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilder.scala:
##########
@@ -386,7 +393,10 @@ object PrivilegesBuilder {
case "DescribeFunctionCommand" =>
val func = getPlanField[FunctionIdentifier]("functionName")
- inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ val isPersistentFunction = spark.sessionState.catalog.isPersistentFunction(func)
+ if (isPersistentFunction) {
+ inputObjs += functionPrivileges(func.database.orNull, func.funcName)
+ }
Review Comment:
```suggestion
val isPersistentFunction =
spark.sessionState.catalog.isPersistentFunction(FunctionIdentifier(funName, db))
if (isPersistentFunction) {
inputObjs += functionPrivileges(db.orNull, funName)
}```
--
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