You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/02/23 13:53:33 UTC
[GitHub] [incubator-kyuubi] SteNicholas opened a new pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
SteNicholas opened a new pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967
### _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.
-->
Make plan only mode skippable for configuable plans.
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [x] [Run test](https://kyuubi.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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813505997
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1148,6 +1148,19 @@ object KyuubiConf {
.checkValues(OperationModes.values.map(_.toString))
.createWithDefault(OperationModes.NONE.toString)
+ val OPERATION_PLAN_ONLY_EXCLUDES: ConfigEntry[Seq[String]] =
+ buildConf("operation.plan.only.excludes")
+ .doc("Comma-separated list of query plan names, in the form of simple class names, i.e, " +
+ "for `set abc=xyz`, the value will be `SetCommand`. For those auxiliary plans, such as " +
+ "`switch databases`, `set properties`, or `create temporary view` e.t.c, " +
+ "which are used for setup evaluating environments for analyzing actual queries, " +
+ "we can use this config to exclude them and let them take effect. " +
+ s"See also ${OPERATION_PLAN_ONLY_MODE.key}.")
+ .version("1.5.0")
+ .stringConf
+ .toSequence()
+ .createWithDefault(Seq("ResetCommand", "SetCommand", "SetNamespaceCommand", "UseStatement"))
Review comment:
@iodone, the point above doesn't make sense. The engine side couldn't do any mapping for the user-defined options.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas closed pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas closed pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813497826
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
@cfmcgrady, the return value of the `session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)` is string sequence. Thus, the above suggestion has something wrong because you map the `Option`.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813508377
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
seesion conf has already been set to spark confs
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
iodone commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813520056
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/PlanOnlyOperationSuite.scala
##########
@@ -79,31 +81,25 @@ class PlanOnlyOperationSuite extends WithKyuubiServer with HiveJDBCTestHelper {
}
}
- test("KYUUBI #1920: Plan only operations with Usestatement or SetNamespaceCommand skiped") {
- withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> NONE.toString))(Map.empty) {
+ test("KYUUBI #1920: Plan only operations with UseStatement or SetNamespaceCommand") {
+ withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY_MODE.key -> NONE.toString))(Map.empty) {
withDatabases("test_database") { statement =>
statement.execute("create database test_database")
- statement.execute(s"set ${KyuubiConf.OPERATION_PLAN_ONLY.key}=optimize")
+ statement.execute(s"set ${KyuubiConf.OPERATION_PLAN_ONLY_MODE.key}=$OPTIMIZE")
val result = statement.executeQuery("use test_database")
assert(!result.next(), "In contrast to PlanOnly mode, it will returns an empty result")
}
}
}
- test("KYUUBI #1920: Plan only operations with CacheTable skiped") {
- withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> OPTIMIZE.toString))(Map.empty) {
- withJdbcStatement() { statement =>
- val result = statement.executeQuery("cache table cached_table as select 1")
- assert(!result.next(), "In contrast to PlanOnly mode, it will returns an empty result")
- }
- }
- }
-
- test("KYUUBI #1920: Plan only operations with CreateViewStatement or CreateViewCommand skiped") {
- withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> OPTIMIZE.toString))(Map.empty) {
+ test("KYUUBI #1920: Plan only operations with CreateViewStatement or CreateViewCommand") {
+ withSessionConf()(
+ Map(KyuubiConf.OPERATION_PLAN_ONLY_EXCLUDES.key -> "CreateViewStatement,CreateViewCommand"))(
+ Map.empty) {
withJdbcStatement() { statement =>
val result = statement.executeQuery("create temp view temp_view as select 1")
Review comment:
https://github.com/apache/incubator-kyuubi/pull/1921/files#r811719564 @cfmcgrady‘s Tips
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#issuecomment-1049421034
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?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 [#1967](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4c4f189) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/1f846aa398a7d7256412b1105544add64bb8c7b5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1f846aa) will **increase** coverage by `0.01%`.
> The diff coverage is `80.00%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1967 +/- ##
============================================
+ Coverage 60.38% 60.40% +0.01%
Complexity 69 69
============================================
Files 305 305
Lines 14859 14863 +4
Branches 1927 1919 -8
============================================
+ Hits 8973 8978 +5
- Misses 5110 5115 +5
+ Partials 776 770 -6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `86.11% <50.00%> (ø)` | |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vU3BhcmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `83.33% <50.00%> (ø)` | |
| [...ubi/engine/spark/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `60.00% <66.66%> (+11.11%)` | :arrow_up: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.26% <100.00%> (-0.10%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `67.46% <0.00%> (-2.41%)` | :arrow_down: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `61.33% <0.00%> (-1.34%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/KyuubiSQLException.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9LeXV1YmlTUUxFeGNlcHRpb24uc2NhbGE=) | `92.77% <0.00%> (-1.21%)` | :arrow_down: |
| [...ala/org/apache/kyuubi/session/SessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL1Nlc3Npb25NYW5hZ2VyLnNjYWxh) | `85.12% <0.00%> (-0.83%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1f846aa...4c4f189](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#issuecomment-1049421034
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?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 [#1967](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4c4f189) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/1f846aa398a7d7256412b1105544add64bb8c7b5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1f846aa) will **increase** coverage by `0.01%`.
> The diff coverage is `80.00%`.
> :exclamation: Current head 4c4f189 differs from pull request most recent head 621fa71. Consider uploading reports for the commit 621fa71 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1967 +/- ##
============================================
+ Coverage 60.38% 60.40% +0.01%
Complexity 69 69
============================================
Files 305 305
Lines 14859 14863 +4
Branches 1927 1919 -8
============================================
+ Hits 8973 8978 +5
- Misses 5110 5115 +5
+ Partials 776 770 -6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `86.11% <50.00%> (ø)` | |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vU3BhcmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `83.33% <50.00%> (ø)` | |
| [...ubi/engine/spark/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `60.00% <66.66%> (+11.11%)` | :arrow_up: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.26% <100.00%> (-0.10%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `67.46% <0.00%> (-2.41%)` | :arrow_down: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `61.33% <0.00%> (-1.34%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/KyuubiSQLException.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9LeXV1YmlTUUxFeGNlcHRpb24uc2NhbGE=) | `92.77% <0.00%> (-1.21%)` | :arrow_down: |
| [...ala/org/apache/kyuubi/session/SessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL1Nlc3Npb25NYW5hZ2VyLnNjYWxh) | `85.12% <0.00%> (-0.83%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1f846aa...621fa71](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813497826
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
@cfmcgrady, the return value of the `session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)` is string sequence.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] cfmcgrady commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
cfmcgrady commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813503708
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
+1 for @ulysses-you 's suggestion.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#issuecomment-1049491424
thanks, 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#issuecomment-1049489054
@yaooqinn, please help to merge.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813523417
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/PlanOnlyOperationSuite.scala
##########
@@ -79,31 +81,25 @@ class PlanOnlyOperationSuite extends WithKyuubiServer with HiveJDBCTestHelper {
}
}
- test("KYUUBI #1920: Plan only operations with Usestatement or SetNamespaceCommand skiped") {
- withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> NONE.toString))(Map.empty) {
+ test("KYUUBI #1920: Plan only operations with UseStatement or SetNamespaceCommand") {
+ withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY_MODE.key -> NONE.toString))(Map.empty) {
withDatabases("test_database") { statement =>
statement.execute("create database test_database")
- statement.execute(s"set ${KyuubiConf.OPERATION_PLAN_ONLY.key}=optimize")
+ statement.execute(s"set ${KyuubiConf.OPERATION_PLAN_ONLY_MODE.key}=$OPTIMIZE")
val result = statement.executeQuery("use test_database")
assert(!result.next(), "In contrast to PlanOnly mode, it will returns an empty result")
}
}
}
- test("KYUUBI #1920: Plan only operations with CacheTable skiped") {
- withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> OPTIMIZE.toString))(Map.empty) {
- withJdbcStatement() { statement =>
- val result = statement.executeQuery("cache table cached_table as select 1")
- assert(!result.next(), "In contrast to PlanOnly mode, it will returns an empty result")
- }
- }
- }
-
- test("KYUUBI #1920: Plan only operations with CreateViewStatement or CreateViewCommand skiped") {
- withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> OPTIMIZE.toString))(Map.empty) {
+ test("KYUUBI #1920: Plan only operations with CreateViewStatement or CreateViewCommand") {
+ withSessionConf()(
+ Map(KyuubiConf.OPERATION_PLAN_ONLY_EXCLUDES.key -> "CreateViewStatement,CreateViewCommand"))(
+ Map.empty) {
withJdbcStatement() { statement =>
val result = statement.executeQuery("create temp view temp_view as select 1")
Review comment:
@iodone, using `statement.execute` could drop the `temp_view`, no need to use `statement.executeQuery` to drop the view.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813509827
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
@ulysses-you, as you mentioned, the [SparkOperation.scala#L73](https://github.com/apache/incubator-kyuubi/blob/master/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/SparkOperation.scala#L73) has the same problem, right?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r812979909
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -32,7 +31,8 @@ import org.apache.kyuubi.session.Session
class PlanOnlyStatement(
session: Session,
override val statement: String,
- mode: OperationMode)
+ mode: OperationMode,
+ skip: Seq[String])
Review comment:
we can move it out of the class parameters to make it simple，like
private val exculdedPlans = spark.conf.getOrElse(xxx, session.getConf.get)
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813491285
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
```suggestion
private val planExcludes: Seq[String] = {
spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key).map( _.split(",").map(_.trim).toSeq)
.getOrElse(session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES))
}
```
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#issuecomment-1049421034
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?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 [#1967](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4c4f189) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/1f846aa398a7d7256412b1105544add64bb8c7b5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1f846aa) will **increase** coverage by `0.01%`.
> The diff coverage is `80.00%`.
> :exclamation: Current head 4c4f189 differs from pull request most recent head b5cff40. Consider uploading reports for the commit b5cff40 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1967 +/- ##
============================================
+ Coverage 60.38% 60.40% +0.01%
Complexity 69 69
============================================
Files 305 305
Lines 14859 14863 +4
Branches 1927 1919 -8
============================================
+ Hits 8973 8978 +5
- Misses 5110 5115 +5
+ Partials 776 770 -6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `86.11% <50.00%> (ø)` | |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vU3BhcmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `83.33% <50.00%> (ø)` | |
| [...ubi/engine/spark/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `60.00% <66.66%> (+11.11%)` | :arrow_up: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.26% <100.00%> (-0.10%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `67.46% <0.00%> (-2.41%)` | :arrow_down: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `61.33% <0.00%> (-1.34%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/KyuubiSQLException.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9LeXV1YmlTUUxFeGNlcHRpb24uc2NhbGE=) | `92.77% <0.00%> (-1.21%)` | :arrow_down: |
| [...ala/org/apache/kyuubi/session/SessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL1Nlc3Npb25NYW5hZ2VyLnNjYWxh) | `85.12% <0.00%> (-0.83%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1933533...b5cff40](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#issuecomment-1049421034
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?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 [#1967](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b5cff40) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/19335334dbcd31733403a06a0ad5a3ee49253ed7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1933533) will **increase** coverage by `0.08%`.
> The diff coverage is `88.88%`.
> :exclamation: Current head b5cff40 differs from pull request most recent head e253aec. Consider uploading reports for the commit e253aec to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1967 +/- ##
============================================
+ Coverage 60.36% 60.44% +0.08%
Complexity 69 69
============================================
Files 305 305
Lines 14859 14862 +3
Branches 1927 1918 -9
============================================
+ Hits 8970 8984 +14
+ Misses 5112 5111 -1
+ Partials 777 767 -10
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `86.11% <50.00%> (ø)` | |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vU3BhcmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `83.33% <50.00%> (ø)` | |
| [...ubi/engine/spark/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `64.10% <100.00%> (+15.21%)` | :arrow_up: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.26% <100.00%> (-0.10%)` | :arrow_down: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `61.33% <0.00%> (-1.34%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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=) | `84.40% <0.00%> (+0.91%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `69.87% <0.00%> (+2.40%)` | :arrow_up: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `95.00% <0.00%> (+2.50%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1933533...e253aec](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] cfmcgrady commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
cfmcgrady commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813500099
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
Yes, you are right, I misread before :)
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] ulysses-you commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813532608
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
I see, we have merged session conf into spark session conf. 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#issuecomment-1048808990
@yaooqinn @pan3793 @iodone, could you please help to review this 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r812967985
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1148,6 +1148,15 @@ object KyuubiConf {
.checkValues(OperationModes.values.map(_.toString))
.createWithDefault(OperationModes.NONE.toString)
+ val OPERATION_PLAN_ONLY_SKIP: ConfigEntry[Seq[String]] =
+ buildConf("operation.plan.only.skip")
+ .doc("Skipped statements or commands in a PARSE, ANALYZE, OPTIMIZE, PHYSICAL, EXECUTION " +
+ "only way without executing the query.")
+ .version("1.5.0")
+ .stringConf
+ .toSequence()
+ .createWithDefault(Nil)
Review comment:
Let add the Plans that match set/reset/use here to keep the backward compatibility and better UX.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] ulysses-you commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813492008
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
the priority of session conf should be higher than spark conf
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#issuecomment-1049421034
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?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 [#1967](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4c4f189) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/1f846aa398a7d7256412b1105544add64bb8c7b5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1f846aa) will **increase** coverage by `0.01%`.
> The diff coverage is `80.00%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1967 +/- ##
============================================
+ Coverage 60.38% 60.40% +0.01%
Complexity 69 69
============================================
Files 305 305
Lines 14859 14863 +4
Branches 1927 1919 -8
============================================
+ Hits 8973 8978 +5
- Misses 5110 5115 +5
+ Partials 776 770 -6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `86.11% <50.00%> (ø)` | |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vU3BhcmtTUUxPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `83.33% <50.00%> (ø)` | |
| [...ubi/engine/spark/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `60.00% <66.66%> (+11.11%)` | :arrow_up: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.26% <100.00%> (-0.10%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `67.46% <0.00%> (-2.41%)` | :arrow_down: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `61.33% <0.00%> (-1.34%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/KyuubiSQLException.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9LeXV1YmlTUUxFeGNlcHRpb24uc2NhbGE=) | `92.77% <0.00%> (-1.21%)` | :arrow_down: |
| [...ala/org/apache/kyuubi/session/SessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL1Nlc3Npb25NYW5hZ2VyLnNjYWxh) | `85.12% <0.00%> (-0.83%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1f846aa...4c4f189](https://codecov.io/gh/apache/incubator-kyuubi/pull/1967?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn closed pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn closed pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813509247
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
SessionManger's getConf is not session conf, its engine conf
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813522749
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/PlanOnlyOperationSuite.scala
##########
@@ -79,31 +81,25 @@ class PlanOnlyOperationSuite extends WithKyuubiServer with HiveJDBCTestHelper {
}
}
- test("KYUUBI #1920: Plan only operations with Usestatement or SetNamespaceCommand skiped") {
- withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> NONE.toString))(Map.empty) {
+ test("KYUUBI #1920: Plan only operations with UseStatement or SetNamespaceCommand") {
+ withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY_MODE.key -> NONE.toString))(Map.empty) {
withDatabases("test_database") { statement =>
statement.execute("create database test_database")
- statement.execute(s"set ${KyuubiConf.OPERATION_PLAN_ONLY.key}=optimize")
+ statement.execute(s"set ${KyuubiConf.OPERATION_PLAN_ONLY_MODE.key}=$OPTIMIZE")
val result = statement.executeQuery("use test_database")
assert(!result.next(), "In contrast to PlanOnly mode, it will returns an empty result")
}
}
}
- test("KYUUBI #1920: Plan only operations with CacheTable skiped") {
- withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> OPTIMIZE.toString))(Map.empty) {
- withJdbcStatement() { statement =>
- val result = statement.executeQuery("cache table cached_table as select 1")
- assert(!result.next(), "In contrast to PlanOnly mode, it will returns an empty result")
- }
- }
- }
-
- test("KYUUBI #1920: Plan only operations with CreateViewStatement or CreateViewCommand skiped") {
- withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> OPTIMIZE.toString))(Map.empty) {
+ test("KYUUBI #1920: Plan only operations with CreateViewStatement or CreateViewCommand") {
+ withSessionConf()(
+ Map(KyuubiConf.OPERATION_PLAN_ONLY_EXCLUDES.key -> "CreateViewStatement,CreateViewCommand"))(
+ Map.empty) {
withJdbcStatement() { statement =>
val result = statement.executeQuery("create temp view temp_view as select 1")
Review comment:
we can play the trick with `withJdbcStatement("view_temp") { ... "create temp view view_temp"}`
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813501623
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
@ulysses-you, IMO, the priority of the spark conf is higher than session conf. cc @yaooqinn
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r812979909
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -32,7 +31,8 @@ import org.apache.kyuubi.session.Session
class PlanOnlyStatement(
session: Session,
override val statement: String,
- mode: OperationMode)
+ mode: OperationMode,
+ skip: Seq[String])
Review comment:
we can move it out of the class parameters to make it simple，like
private val excludedPlans = spark.conf.getOrElse(xxx, session.getConf.get)
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
iodone commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813504059
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1148,6 +1148,19 @@ object KyuubiConf {
.checkValues(OperationModes.values.map(_.toString))
.createWithDefault(OperationModes.NONE.toString)
+ val OPERATION_PLAN_ONLY_EXCLUDES: ConfigEntry[Seq[String]] =
+ buildConf("operation.plan.only.excludes")
+ .doc("Comma-separated list of query plan names, in the form of simple class names, i.e, " +
+ "for `set abc=xyz`, the value will be `SetCommand`. For those auxiliary plans, such as " +
+ "`switch databases`, `set properties`, or `create temporary view` e.t.c, " +
+ "which are used for setup evaluating environments for analyzing actual queries, " +
+ "we can use this config to exclude them and let them take effect. " +
+ s"See also ${OPERATION_PLAN_ONLY_MODE.key}.")
+ .version("1.5.0")
+ .stringConf
+ .toSequence()
+ .createWithDefault(Seq("ResetCommand", "SetCommand", "SetNamespaceCommand", "UseStatement"))
Review comment:
IMO, the configuration here is to provide the exact className, if you want to skip cacheTable,
`CacheTableStatement` in spark 3.0 and `CacheTableCommand` in Spark 3.2, it's too hard for users to set the right options. Could the options here be more generic, like set/reset/cacheTable/CreateTempView, rather than requiring the actual class name?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] iodone commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
iodone commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813503450
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -46,24 +51,11 @@ class PlanOnlyStatement(
} else result.schema
}
- private def shouldDirectRun(plan: LogicalPlan): Boolean = {
- val className = plan.getClass.getSimpleName
- className == "SetCommand" ||
- className == "ResetCommand" ||
- className == "UseStatement" ||
- className == "SetNamespaceCommand" ||
- className == "CacheTableStatement" ||
- className == "CacheTableCommand" ||
- className == "CacheTableAsSelect" ||
- className == "CreateViewStatement" ||
- className == "CreateViewCommand"
- }
-
override protected def runInternal(): Unit = {
try {
val parsed = spark.sessionState.sqlParser.parsePlan(statement)
parsed match {
- case cmd if shouldDirectRun(cmd) =>
+ case cmd if planExcludes.contains(cmd.getClass.getSimpleName) =>
Review comment:
IMO, the configuration here is to provide the exact className, if you want to skip cacheTable,
`CacheTableStatement` in spark 3.0 and `CacheTableCommand` in Spark 3.2, it's too hard for users to set the right options. Could the options here be more generic, like set/reset/cacheTable/CreateTempView, rather than requiring the actual class name?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813505213
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
@yaooqinn, does this suggestion is wrong? As below mentioned, the priority of session conf should be higher than spark conf.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r812952524
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1148,6 +1148,15 @@ object KyuubiConf {
.checkValues(OperationModes.values.map(_.toString))
.createWithDefault(OperationModes.NONE.toString)
+ val OPERATION_PLAN_ONLY_SKIP: ConfigEntry[Seq[String]] =
+ buildConf("operation.plan.only.skip")
Review comment:
operation.plan.only.excludes
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] SteNicholas removed a comment on pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
SteNicholas removed a comment on pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#issuecomment-1048808990
@yaooqinn @pan3793 @iodone, could you please help to review this 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] cfmcgrady commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
cfmcgrady commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r813495717
##########
File path: externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/PlanOnlyStatement.scala
##########
@@ -36,6 +36,11 @@ class PlanOnlyStatement(
extends SparkOperation(OperationType.EXECUTE_STATEMENT, session) {
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ private val configExcludes: Option[String] =
+ spark.conf.getOption(OPERATION_PLAN_ONLY_EXCLUDES.key)
+ private val planExcludes: Seq[String] =
+ if (configExcludes.isEmpty) session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)
+ else configExcludes.get.split(",").toSeq
Review comment:
we need to split the session conf `session.sessionManager.getConf.get(OPERATION_PLAN_ONLY_EXCLUDES)` by comma as well.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1967: [KYUUBI #1963] Make plan only mode skippable for configuable plans
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1967:
URL: https://github.com/apache/incubator-kyuubi/pull/1967#discussion_r812966511
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala
##########
@@ -1148,6 +1148,15 @@ object KyuubiConf {
.checkValues(OperationModes.values.map(_.toString))
.createWithDefault(OperationModes.NONE.toString)
+ val OPERATION_PLAN_ONLY_SKIP: ConfigEntry[Seq[String]] =
+ buildConf("operation.plan.only.skip")
+ .doc("Skipped statements or commands in a PARSE, ANALYZE, OPTIMIZE, PHYSICAL, EXECUTION " +
Review comment:
Comma-separated list of query plan names, in the form of simple class names, i.e, for `set abc=xyz`, the value will be `SetCommand`. For those auxiliary plans, such as `switch databases`, `set properties`, or `create temporary view` e.t.c, which are used for setup evaluating environments for analyzing actual queries, we can use this config to exclude them and let them take effect. See also `kyuubi.operation.plan.only.mode`.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org