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/10 18:39:12 UTC
[GitHub] [incubator-kyuubi] SteNicholas opened a new pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
SteNicholas opened a new pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891
<!--
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/contributions.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?_
Support `PlanOnlyStatement` operation like Spark SQL engine.
### _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 pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038659169
@pan3793, thanks for your detailed review. I have addressed above comments. Please help to review again.
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805618457
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/FlinkSQLOperationManager.scala
##########
@@ -18,22 +18,38 @@
package org.apache.kyuubi.engine.flink.operation
import java.util
+import java.util.Locale
-import scala.collection.JavaConverters.asScalaBufferConverter
+import scala.collection.JavaConverters._
+import org.apache.kyuubi.config.KyuubiConf._
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
import org.apache.kyuubi.engine.flink.result.Constants
+import org.apache.kyuubi.engine.flink.session.FlinkSessionImpl
import org.apache.kyuubi.operation.{Operation, OperationManager}
import org.apache.kyuubi.session.Session
class FlinkSQLOperationManager extends OperationManager("FlinkSQLOperationManager") {
+ private lazy val operationModeDefault = getConf.get(OPERATION_PLAN_ONLY)
+
override def newExecuteStatementOperation(
session: Session,
statement: String,
confOverlay: Map[String, String],
runAsync: Boolean,
queryTimeout: Long): Operation = {
- val op = new ExecuteStatement(session, statement, runAsync, queryTimeout)
+ val flinkSession = session.asInstanceOf[FlinkSessionImpl]
+ val mode =
+ flinkSession.sessionContext.getConfigMap.getOrDefault(
+ OPERATION_PLAN_ONLY.key,
+ flinkSession.normalizedConf.getOrElse(OPERATION_PLAN_ONLY.key, operationModeDefault))
Review comment:
this is a bug, session configs shall be set first in `flinkSession` for sharing across operations within a session.
We shall override `open` in `FlinkSessionImpl` and call it in `org.apache.kyuubi.engine.flink.session.FlinkSQLSessionManager#openSession`, which worth a separate PR to fix
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038569464
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 [#1891](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (29ba3dd) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/216dd66a4703b6e72989582a8106afc4b8703795?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (216dd66) will **increase** coverage by `0.12%`.
> The diff coverage is `62.50%`.
> :exclamation: Current head 29ba3dd differs from pull request most recent head 12dcf76. Consider uploading reports for the commit 12dcf76 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 #1891 +/- ##
============================================
+ Coverage 59.70% 59.83% +0.12%
- Complexity 269 341 +72
============================================
Files 282 292 +10
Lines 13978 14301 +323
Branches 1781 1812 +31
============================================
+ Hits 8346 8557 +211
- Misses 4924 5008 +84
- Partials 708 736 +28
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ubi/engine/flink/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `55.76% <55.76%> (ø)` | |
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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) | `85.00% <76.47%> (-6.67%)` | :arrow_down: |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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% <100.00%> (+0.40%)` | :arrow_up: |
| [.../org/apache/kyuubi/engine/trino/TrinoContext.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vQ29udGV4dC5zY2FsYQ==) | `83.33% <0.00%> (-16.67%)` | :arrow_down: |
| [...ache/kyuubi/engine/spark/events/SessionEvent.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvU2Vzc2lvbkV2ZW50LnNjYWxh) | `79.16% <0.00%> (-7.79%)` | :arrow_down: |
| [...kyuubi/engine/spark/events/EngineEventsStore.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvRW5naW5lRXZlbnRzU3RvcmUuc2NhbGE=) | `80.00% <0.00%> (-6.67%)` | :arrow_down: |
| [...uubi/engine/spark/events/SparkOperationEvent.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvU3BhcmtPcGVyYXRpb25FdmVudC5zY2FsYQ==) | `86.48% <0.00%> (-2.41%)` | :arrow_down: |
| [...g/apache/spark/kyuubi/SparkSQLEngineListener.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsva3l1dWJpL1NwYXJrU1FMRW5naW5lTGlzdGVuZXIuc2NhbGE=) | `86.27% <0.00%> (-1.87%)` | :arrow_down: |
| [...src/main/scala/org/apache/spark/ui/EngineTab.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3BhcmsvdWkvRW5naW5lVGFiLnNjYWxh) | `70.73% <0.00%> (-1.69%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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) | `65.06% <0.00%> (-0.83%)` | :arrow_down: |
| ... and [26 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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/1891?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 [216dd66...12dcf76](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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] yanghua commented on a change in pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805665914
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
##########
@@ -107,10 +105,13 @@ class ExecuteStatement(
val operation = executor.parseStatement(sessionId, statement)
operation match {
case queryOperation: QueryOperation => runQueryOperation(queryOperation)
- case setOperation: SetOperation => runSetOperation(setOperation)
- case resetOperation: ResetOperation => runResetOperation(resetOperation)
+ case setOperation: SetOperation =>
Review comment:
You mean `SparkSQLOperationManager#newExecuteStatementOperation`？
> in SparkSQLOperationManager, the ExecuteStatement and PlanOnlyStatement both support the SetOperation and ResetOperation.
where？
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038569464
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 [#1891](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (29ba3dd) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/a6480d0e31b72ccca0e8f2a0aa49bfd6f0e8f36e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a6480d0) will **increase** coverage by `0.07%`.
> The diff coverage is `62.50%`.
> :exclamation: Current head 29ba3dd differs from pull request most recent head b0550c0. Consider uploading reports for the commit b0550c0 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 #1891 +/- ##
============================================
+ Coverage 59.75% 59.83% +0.07%
- Complexity 321 341 +20
============================================
Files 291 292 +1
Lines 14234 14301 +67
Branches 1806 1812 +6
============================================
+ Hits 8506 8557 +51
- Misses 5003 5008 +5
- Partials 725 736 +11
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ubi/engine/flink/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `55.76% <55.76%> (ø)` | |
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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) | `85.00% <76.47%> (-6.67%)` | :arrow_down: |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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% <100.00%> (+0.40%)` | :arrow_up: |
| [...rc/main/scala/org/apache/spark/ui/EnginePage.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3BhcmsvdWkvRW5naW5lUGFnZS5zY2FsYQ==) | `79.45% <0.00%> (+0.47%)` | :arrow_up: |
| [...kyuubi/engine/flink/operation/FlinkOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtPcGVyYXRpb24uc2NhbGE=) | `69.35% <0.00%> (+14.51%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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/1891?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 [a6480d0...b0550c0](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1035924983
> @SteNicholas Please check the CI?
@yanghua, sorry for the failure of the CI. I have fixed the problem for the failure. Please 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] codecov-commenter edited a comment on pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038569464
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 [#1891](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0e256ac) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/a6480d0e31b72ccca0e8f2a0aa49bfd6f0e8f36e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a6480d0) will **increase** coverage by `0.13%`.
> The diff coverage is `80.55%`.
> :exclamation: Current head 0e256ac differs from pull request most recent head 0840139. Consider uploading reports for the commit 0840139 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 #1891 +/- ##
============================================
+ Coverage 59.75% 59.89% +0.13%
- Complexity 321 332 +11
============================================
Files 291 292 +1
Lines 14234 14267 +33
Branches 1806 1813 +7
============================================
+ Hits 8506 8545 +39
+ Misses 5003 4991 -12
- Partials 725 731 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...che/kyuubi/engine/flink/result/ResultSetUtil.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9yZXN1bHQvUmVzdWx0U2V0VXRpbC5zY2FsYQ==) | `80.48% <73.33%> (-19.52%)` | :arrow_down: |
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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) | `87.50% <77.77%> (-4.17%)` | :arrow_down: |
| [...ubi/engine/flink/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `84.00% <84.00%> (ø)` | |
| [...uubi/engine/flink/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `81.08% <100.00%> (+2.98%)` | :arrow_up: |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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% <100.00%> (+0.40%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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.40%)` | :arrow_up: |
| [...kyuubi/engine/flink/operation/FlinkOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtPcGVyYXRpb24uc2NhbGE=) | `69.35% <0.00%> (+14.51%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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/1891?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 [a6480d0...0840139](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 commented on a change in pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r807603220
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
##########
@@ -107,10 +105,13 @@ class ExecuteStatement(
val operation = executor.parseStatement(sessionId, statement)
operation match {
case queryOperation: QueryOperation => runQueryOperation(queryOperation)
- case setOperation: SetOperation => runSetOperation(setOperation)
- case resetOperation: ResetOperation => runResetOperation(resetOperation)
+ case setOperation: SetOperation =>
Review comment:
> @yanghua, I mean that in `SparkSQLOperationManager#newExecuteStatementOperation`, the `SetOperation` and `ResetOperation` are both supported in `PlanOnlyStatement` and `ExecuteStatement`. The behavior you mentioned above could be implemented in future. cc @yaooqinn
make sense to me
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038702181
@pan3793, thanks for your detailed review. I have addressed above comments. Please help to review again.
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805594293
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/FlinkSQLOperationManager.scala
##########
@@ -18,22 +18,38 @@
package org.apache.kyuubi.engine.flink.operation
import java.util
+import java.util.Locale
-import scala.collection.JavaConverters.asScalaBufferConverter
+import scala.collection.JavaConverters._
+import org.apache.kyuubi.config.KyuubiConf._
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
import org.apache.kyuubi.engine.flink.result.Constants
+import org.apache.kyuubi.engine.flink.session.FlinkSessionImpl
import org.apache.kyuubi.operation.{Operation, OperationManager}
import org.apache.kyuubi.session.Session
class FlinkSQLOperationManager extends OperationManager("FlinkSQLOperationManager") {
+ private lazy val operationModeDefault = getConf.get(OPERATION_PLAN_ONLY)
+
override def newExecuteStatementOperation(
session: Session,
statement: String,
confOverlay: Map[String, String],
runAsync: Boolean,
queryTimeout: Long): Operation = {
- val op = new ExecuteStatement(session, statement, runAsync, queryTimeout)
+ val flinkSession = session.asInstanceOf[FlinkSessionImpl]
+ val mode =
+ flinkSession.sessionContext.getConfigMap.getOrDefault(
+ OPERATION_PLAN_ONLY.key,
+ flinkSession.normalizedConf.getOrElse(OPERATION_PLAN_ONLY.key, operationModeDefault))
Review comment:
tuples in ` flinkSession.normalizedConf` shall be already in `flinkSession.sessionContext.getConfigMap`
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805618457
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/FlinkSQLOperationManager.scala
##########
@@ -18,22 +18,38 @@
package org.apache.kyuubi.engine.flink.operation
import java.util
+import java.util.Locale
-import scala.collection.JavaConverters.asScalaBufferConverter
+import scala.collection.JavaConverters._
+import org.apache.kyuubi.config.KyuubiConf._
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
import org.apache.kyuubi.engine.flink.result.Constants
+import org.apache.kyuubi.engine.flink.session.FlinkSessionImpl
import org.apache.kyuubi.operation.{Operation, OperationManager}
import org.apache.kyuubi.session.Session
class FlinkSQLOperationManager extends OperationManager("FlinkSQLOperationManager") {
+ private lazy val operationModeDefault = getConf.get(OPERATION_PLAN_ONLY)
+
override def newExecuteStatementOperation(
session: Session,
statement: String,
confOverlay: Map[String, String],
runAsync: Boolean,
queryTimeout: Long): Operation = {
- val op = new ExecuteStatement(session, statement, runAsync, queryTimeout)
+ val flinkSession = session.asInstanceOf[FlinkSessionImpl]
+ val mode =
+ flinkSession.sessionContext.getConfigMap.getOrDefault(
+ OPERATION_PLAN_ONLY.key,
+ flinkSession.normalizedConf.getOrElse(OPERATION_PLAN_ONLY.key, operationModeDefault))
Review comment:
this is a bug, session configs shall be set first in `flinkSession` for share across operations within a session.
We shall override `open` in `FlinkSessionImpl` and call it in `org.apache.kyuubi.engine.flink.session.FlinkSQLSessionManager#openSession`, which worth a separate PR to fix
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1041198281
@yaooqinn, thanks for your review. I have addressed your comments and bugfix the `FlinkSQLSessionManager#operSession`. Please help to review again.
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas removed a comment on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038691153
@pan3793, thanks for your detailed review. I have addressed above comments. Please help to review again.
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805655440
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/FlinkSQLOperationManager.scala
##########
@@ -18,22 +18,38 @@
package org.apache.kyuubi.engine.flink.operation
import java.util
+import java.util.Locale
-import scala.collection.JavaConverters.asScalaBufferConverter
+import scala.collection.JavaConverters._
+import org.apache.kyuubi.config.KyuubiConf._
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
import org.apache.kyuubi.engine.flink.result.Constants
+import org.apache.kyuubi.engine.flink.session.FlinkSessionImpl
import org.apache.kyuubi.operation.{Operation, OperationManager}
import org.apache.kyuubi.session.Session
class FlinkSQLOperationManager extends OperationManager("FlinkSQLOperationManager") {
+ private lazy val operationModeDefault = getConf.get(OPERATION_PLAN_ONLY)
+
override def newExecuteStatementOperation(
session: Session,
statement: String,
confOverlay: Map[String, String],
runAsync: Boolean,
queryTimeout: Long): Operation = {
- val op = new ExecuteStatement(session, statement, runAsync, queryTimeout)
+ val flinkSession = session.asInstanceOf[FlinkSessionImpl]
+ val mode =
+ flinkSession.sessionContext.getConfigMap.getOrDefault(
+ OPERATION_PLAN_ONLY.key,
+ flinkSession.normalizedConf.getOrElse(OPERATION_PLAN_ONLY.key, operationModeDefault))
Review comment:
> IMO, current implementation is correct.
>
Not exactly,
If we don't change(merge these configs) there first,
before some `SetOperation(0)`s, the `flinkSession.normalizedConf(1)` shall update `flinkSession.sessionContext.getConfigMap(2)`, the read-side performs (1) -> (2)
After `SetOperation`s, now (2) itself contains newer settings from (0). the read-side perfroms (0) -> (1) -> (2), a.k.a a modified (2) -> (1).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yanghua commented on a change in pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805593187
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
##########
@@ -107,10 +105,13 @@ class ExecuteStatement(
val operation = executor.parseStatement(sessionId, statement)
operation match {
case queryOperation: QueryOperation => runQueryOperation(queryOperation)
- case setOperation: SetOperation => runSetOperation(setOperation)
- case resetOperation: ResetOperation => runResetOperation(resetOperation)
+ case setOperation: SetOperation =>
Review comment:
I mean can we support `SetOperation ` and `ResetOperation ` only in `PlanOnlyStatement `. And remove here.
--
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] yanghua closed pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yanghua closed pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805596263
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/FlinkSQLOperationManager.scala
##########
@@ -18,22 +18,38 @@
package org.apache.kyuubi.engine.flink.operation
import java.util
+import java.util.Locale
-import scala.collection.JavaConverters.asScalaBufferConverter
+import scala.collection.JavaConverters._
+import org.apache.kyuubi.config.KyuubiConf._
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
import org.apache.kyuubi.engine.flink.result.Constants
+import org.apache.kyuubi.engine.flink.session.FlinkSessionImpl
import org.apache.kyuubi.operation.{Operation, OperationManager}
import org.apache.kyuubi.session.Session
class FlinkSQLOperationManager extends OperationManager("FlinkSQLOperationManager") {
+ private lazy val operationModeDefault = getConf.get(OPERATION_PLAN_ONLY)
+
override def newExecuteStatementOperation(
session: Session,
statement: String,
confOverlay: Map[String, String],
runAsync: Boolean,
queryTimeout: Long): Operation = {
- val op = new ExecuteStatement(session, statement, runAsync, queryTimeout)
+ val flinkSession = session.asInstanceOf[FlinkSessionImpl]
+ val mode =
+ flinkSession.sessionContext.getConfigMap.getOrDefault(
+ OPERATION_PLAN_ONLY.key,
+ flinkSession.normalizedConf.getOrElse(OPERATION_PLAN_ONLY.key, operationModeDefault))
Review comment:
@yaooqinn, according to local debug, the `flinkSession.normalizedConf` isn't already in `flinkSession.sessionContext.getConfigMap`.
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805644602
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/FlinkSQLOperationManager.scala
##########
@@ -18,22 +18,38 @@
package org.apache.kyuubi.engine.flink.operation
import java.util
+import java.util.Locale
-import scala.collection.JavaConverters.asScalaBufferConverter
+import scala.collection.JavaConverters._
+import org.apache.kyuubi.config.KyuubiConf._
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
import org.apache.kyuubi.engine.flink.result.Constants
+import org.apache.kyuubi.engine.flink.session.FlinkSessionImpl
import org.apache.kyuubi.operation.{Operation, OperationManager}
import org.apache.kyuubi.session.Session
class FlinkSQLOperationManager extends OperationManager("FlinkSQLOperationManager") {
+ private lazy val operationModeDefault = getConf.get(OPERATION_PLAN_ONLY)
+
override def newExecuteStatementOperation(
session: Session,
statement: String,
confOverlay: Map[String, String],
runAsync: Boolean,
queryTimeout: Long): Operation = {
- val op = new ExecuteStatement(session, statement, runAsync, queryTimeout)
+ val flinkSession = session.asInstanceOf[FlinkSessionImpl]
+ val mode =
+ flinkSession.sessionContext.getConfigMap.getOrDefault(
+ OPERATION_PLAN_ONLY.key,
+ flinkSession.normalizedConf.getOrElse(OPERATION_PLAN_ONLY.key, operationModeDefault))
Review comment:
@yaooqinn, according to your points, does this fix in this pull request? If not, IMO, current implementation is correct.
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038569464
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 [#1891](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0e256ac) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/a6480d0e31b72ccca0e8f2a0aa49bfd6f0e8f36e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a6480d0) will **increase** coverage by `0.13%`.
> The diff coverage is `80.55%`.
> :exclamation: Current head 0e256ac differs from pull request most recent head 214ba56. Consider uploading reports for the commit 214ba56 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 #1891 +/- ##
============================================
+ Coverage 59.75% 59.89% +0.13%
- Complexity 321 332 +11
============================================
Files 291 292 +1
Lines 14234 14267 +33
Branches 1806 1813 +7
============================================
+ Hits 8506 8545 +39
+ Misses 5003 4991 -12
- Partials 725 731 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...che/kyuubi/engine/flink/result/ResultSetUtil.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9yZXN1bHQvUmVzdWx0U2V0VXRpbC5zY2FsYQ==) | `80.48% <73.33%> (-19.52%)` | :arrow_down: |
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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) | `87.50% <77.77%> (-4.17%)` | :arrow_down: |
| [...ubi/engine/flink/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `84.00% <84.00%> (ø)` | |
| [...uubi/engine/flink/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `81.08% <100.00%> (+2.98%)` | :arrow_up: |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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% <100.00%> (+0.40%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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.40%)` | :arrow_up: |
| [...kyuubi/engine/flink/operation/FlinkOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtPcGVyYXRpb24uc2NhbGE=) | `69.35% <0.00%> (+14.51%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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/1891?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 [a6480d0...214ba56](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 commented on a change in pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805655440
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/FlinkSQLOperationManager.scala
##########
@@ -18,22 +18,38 @@
package org.apache.kyuubi.engine.flink.operation
import java.util
+import java.util.Locale
-import scala.collection.JavaConverters.asScalaBufferConverter
+import scala.collection.JavaConverters._
+import org.apache.kyuubi.config.KyuubiConf._
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
import org.apache.kyuubi.engine.flink.result.Constants
+import org.apache.kyuubi.engine.flink.session.FlinkSessionImpl
import org.apache.kyuubi.operation.{Operation, OperationManager}
import org.apache.kyuubi.session.Session
class FlinkSQLOperationManager extends OperationManager("FlinkSQLOperationManager") {
+ private lazy val operationModeDefault = getConf.get(OPERATION_PLAN_ONLY)
+
override def newExecuteStatementOperation(
session: Session,
statement: String,
confOverlay: Map[String, String],
runAsync: Boolean,
queryTimeout: Long): Operation = {
- val op = new ExecuteStatement(session, statement, runAsync, queryTimeout)
+ val flinkSession = session.asInstanceOf[FlinkSessionImpl]
+ val mode =
+ flinkSession.sessionContext.getConfigMap.getOrDefault(
+ OPERATION_PLAN_ONLY.key,
+ flinkSession.normalizedConf.getOrElse(OPERATION_PLAN_ONLY.key, operationModeDefault))
Review comment:
> IMO, current implementation is correct.
>
Not exactly,
If we don't change(merge these configs) there first,
before some `SetOperation(0)`s, as the `flinkSession.normalizedConf(1)` shall update `flinkSession.sessionContext.getConfigMap(2)`, the read-side performs (1) -> (2).
After `SetOperation`s, now (2) itself contains newer settings from (0). the read-side perfroms (0) -> (1) -> (2), a.k.a a modified (2) -> (1).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r807601975
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
##########
@@ -107,10 +105,13 @@ class ExecuteStatement(
val operation = executor.parseStatement(sessionId, statement)
operation match {
case queryOperation: QueryOperation => runQueryOperation(queryOperation)
- case setOperation: SetOperation => runSetOperation(setOperation)
- case resetOperation: ResetOperation => runResetOperation(resetOperation)
+ case setOperation: SetOperation =>
Review comment:
@yanghua, I mean that in `SparkSQLOperationManager#newExecuteStatementOperation`, the `SetOperation` and `ResetOperation` are both supported in `PlanOnlyStatement` and `ExecuteStatement`. The behavior you mentioned above could be implemented in future.
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] SteNicholas commented on pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1041234009
@yaooqinn @pan3793, could you 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] codecov-commenter edited a comment on pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038569464
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 [#1891](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (29ba3dd) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/a6480d0e31b72ccca0e8f2a0aa49bfd6f0e8f36e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a6480d0) will **increase** coverage by `0.07%`.
> The diff coverage is `62.50%`.
> :exclamation: Current head 29ba3dd differs from pull request most recent head 0840139. Consider uploading reports for the commit 0840139 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 #1891 +/- ##
============================================
+ Coverage 59.75% 59.83% +0.07%
- Complexity 321 341 +20
============================================
Files 291 292 +1
Lines 14234 14301 +67
Branches 1806 1812 +6
============================================
+ Hits 8506 8557 +51
- Misses 5003 5008 +5
- Partials 725 736 +11
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ubi/engine/flink/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `55.76% <55.76%> (ø)` | |
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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) | `85.00% <76.47%> (-6.67%)` | :arrow_down: |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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% <100.00%> (+0.40%)` | :arrow_up: |
| [...rc/main/scala/org/apache/spark/ui/EnginePage.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3BhcmsvdWkvRW5naW5lUGFnZS5zY2FsYQ==) | `79.45% <0.00%> (+0.47%)` | :arrow_up: |
| [...kyuubi/engine/flink/operation/FlinkOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtPcGVyYXRpb24uc2NhbGE=) | `69.35% <0.00%> (+14.51%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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/1891?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 [a6480d0...0840139](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 commented on a change in pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r804413991
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/FlinkSQLOperationManager.scala
##########
@@ -18,22 +18,48 @@
package org.apache.kyuubi.engine.flink.operation
import java.util
+import java.util.Locale
-import scala.collection.JavaConverters.asScalaBufferConverter
+import scala.collection.JavaConverters._
+import org.apache.kyuubi.config.KyuubiConf._
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
import org.apache.kyuubi.engine.flink.result.Constants
+import org.apache.kyuubi.engine.flink.session.FlinkSessionImpl
import org.apache.kyuubi.operation.{Operation, OperationManager}
import org.apache.kyuubi.session.Session
class FlinkSQLOperationManager extends OperationManager("FlinkSQLOperationManager") {
+ private lazy val operationModeDefault = getConf.get(OPERATION_PLAN_ONLY)
+ private lazy val operationLanguageDefault = getConf.get(OPERATION_LANGUAGE)
+
override def newExecuteStatementOperation(
session: Session,
statement: String,
confOverlay: Map[String, String],
runAsync: Boolean,
queryTimeout: Long): Operation = {
- val op = new ExecuteStatement(session, statement, runAsync, queryTimeout)
+ val flinkSession = session.asInstanceOf[FlinkSessionImpl]
+ val lang = confOverlay.getOrElse(
Review comment:
we can remove it, for now, it will cause scala.MatchError
--
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] yanghua commented on a change in pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r804427821
##########
File path: externals/kyuubi-flink-sql-engine/src/test/scala/org/apache/kyuubi/engine/flink/operation/PlanOnlyOperationSuite.scala
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.flink.operation
+
+import java.sql.Statement
+
+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
+import org.apache.kyuubi.engine.flink.WithFlinkSQLEngine
+import org.apache.kyuubi.operation.HiveJDBCTestHelper
+
+class PlanOnlyOperationSuite extends WithFlinkSQLEngine with HiveJDBCTestHelper {
+
+ override def withKyuubiConf: Map[String, String] =
+ Map(
+ KyuubiConf.ENGINE_SHARE_LEVEL.key -> "user",
+ KyuubiConf.OPERATION_PLAN_ONLY.key -> PARSE.toString,
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUBDOMAIN.key -> "plan-only")
+
+ override protected def jdbcUrl: String =
+ s"jdbc:hive2://${engine.frontendServices.head.connectionUrl}/;"
+
+ test("Plan only operation with system defaults") {
+ withJdbcStatement() { statement =>
+ testPlanOnlyStatement(statement)
+ }
+ }
+
+ test("Plan only operation with with session conf") {
Review comment:
with with
##########
File path: externals/kyuubi-flink-sql-engine/src/test/scala/org/apache/kyuubi/engine/flink/operation/PlanOnlyOperationSuite.scala
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.flink.operation
+
+import java.sql.Statement
+
+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
+import org.apache.kyuubi.engine.flink.WithFlinkSQLEngine
+import org.apache.kyuubi.operation.HiveJDBCTestHelper
+
+class PlanOnlyOperationSuite extends WithFlinkSQLEngine with HiveJDBCTestHelper {
+
+ override def withKyuubiConf: Map[String, String] =
+ Map(
+ KyuubiConf.ENGINE_SHARE_LEVEL.key -> "user",
+ KyuubiConf.OPERATION_PLAN_ONLY.key -> PARSE.toString,
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUBDOMAIN.key -> "plan-only")
+
+ override protected def jdbcUrl: String =
+ s"jdbc:hive2://${engine.frontendServices.head.connectionUrl}/;"
+
+ test("Plan only operation with system defaults") {
+ withJdbcStatement() { statement =>
+ testPlanOnlyStatement(statement)
+ }
+ }
+
+ test("Plan only operation with with session conf") {
+ withSessionConf()(Map(KyuubiConf.OPERATION_PLAN_ONLY.key -> ANALYZE.toString))(Map.empty) {
+ withJdbcStatement() { statement =>
+ val exceptionMsg = intercept[Exception](statement.executeQuery("select 1")).getMessage
+ assert(exceptionMsg.contains(
+ s"The operation mode ${ANALYZE.toString} doesn't support in Flink SQL engine."))
+ }
+ }
+ }
+
+ test("Plan only operation with with set command") {
Review comment:
ditto
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/PlanOnlyStatement.scala
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.flink.operation
+
+import java.util
+
+import scala.collection.mutable.ArrayBuffer
+
+import org.apache.flink.table.api.{DataTypes, ResultKind, TableEnvironment}
+import org.apache.flink.table.catalog.Column
+import org.apache.flink.table.operations.command.{ResetOperation, SetOperation}
+import org.apache.flink.types.Row
+
+import org.apache.kyuubi.KyuubiSQLException
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
+import org.apache.kyuubi.engine.flink.result.{ResultSet, ResultSetUtil}
+import org.apache.kyuubi.operation.OperationType
+import org.apache.kyuubi.operation.log.OperationLog
+import org.apache.kyuubi.session.Session
+
+/**
+ * Perform the statement parsing, analyzing or optimizing only without executing it
+ */
+class PlanOnlyStatement(
+ session: Session,
+ override val statement: String,
+ mode: OperationMode)
+ extends FlinkOperation(OperationType.EXECUTE_STATEMENT, session) {
+
+ private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ override def getOperationLog: Option[OperationLog] = Option(operationLog)
+
+ override protected def runInternal(): Unit = {
+ try {
+ val operation = executor.parseStatement(sessionId, statement)
+ operation match {
+ case setOperation: SetOperation => runSetOperation(setOperation)
+ case resetOperation: ResetOperation => runResetOperation(resetOperation)
+ case _ => explainOperation(statement)
+ }
+ } catch {
+ onError()
+ }
+ }
+
+ private def runSetOperation(setOperation: SetOperation): Unit = {
Review comment:
This is duplicated with `ExecuteStatement#runSetOperation `?
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038691153
@pan3793, thanks for your detailed review. I have addressed above comments. Please help to review again.
--
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] yanghua commented on a change in pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805628247
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
##########
@@ -107,10 +105,13 @@ class ExecuteStatement(
val operation = executor.parseStatement(sessionId, statement)
operation match {
case queryOperation: QueryOperation => runQueryOperation(queryOperation)
- case setOperation: SetOperation => runSetOperation(setOperation)
- case resetOperation: ResetOperation => runResetOperation(resetOperation)
+ case setOperation: SetOperation =>
Review comment:
You have supported these two operations in `PlanOnlyStatement `, right? Maybe here, we can throw an exception then guide users to use `PlanOnlyStatement`, and make the `ExecuteStatement` operation, only support DML and DDL? I am not sure if we need to provide two entry points for this ability.
--
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] yanghua commented on pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1035843481
@SteNicholas Please check the CI?
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1035316497
@yaooqinn @pan3793 @yanghua, 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] codecov-commenter edited a comment on pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038569464
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 [#1891](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3d1c852) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/a6480d0e31b72ccca0e8f2a0aa49bfd6f0e8f36e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a6480d0) will **increase** coverage by `0.11%`.
> The diff coverage is `80.55%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 #1891 +/- ##
============================================
+ Coverage 59.75% 59.87% +0.11%
- Complexity 321 331 +10
============================================
Files 291 292 +1
Lines 14234 14267 +33
Branches 1806 1813 +7
============================================
+ Hits 8506 8542 +36
+ Misses 5003 4992 -11
- Partials 725 733 +8
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...che/kyuubi/engine/flink/result/ResultSetUtil.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9yZXN1bHQvUmVzdWx0U2V0VXRpbC5zY2FsYQ==) | `80.48% <73.33%> (-19.52%)` | :arrow_down: |
| [...ine/flink/operation/FlinkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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) | `87.50% <77.77%> (-4.17%)` | :arrow_down: |
| [...ubi/engine/flink/operation/PlanOnlyStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vUGxhbk9ubHlTdGF0ZW1lbnQuc2NhbGE=) | `84.00% <84.00%> (ø)` | |
| [...uubi/engine/flink/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `81.08% <100.00%> (+2.98%)` | :arrow_up: |
| [...ine/spark/operation/SparkSQLOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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% <100.00%> (+0.40%)` | :arrow_up: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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==) | `92.50% <0.00%> (-2.50%)` | :arrow_down: |
| [...kyuubi/engine/flink/operation/FlinkOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891/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-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9vcGVyYXRpb24vRmxpbmtPcGVyYXRpb24uc2NhbGE=) | `69.35% <0.00%> (+14.51%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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/1891?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 [a6480d0...3d1c852](https://codecov.io/gh/apache/incubator-kyuubi/pull/1891?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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805643664
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
##########
@@ -107,10 +105,13 @@ class ExecuteStatement(
val operation = executor.parseStatement(sessionId, statement)
operation match {
case queryOperation: QueryOperation => runQueryOperation(queryOperation)
- case setOperation: SetOperation => runSetOperation(setOperation)
- case resetOperation: ResetOperation => runResetOperation(resetOperation)
+ case setOperation: SetOperation =>
Review comment:
@yanghua, in `SparkSQLOperationManager`, the `ExecuteStatement` and `PlanOnlyStatement` both support the `SetOperation` and `ResetOperation`. Refer to this, I perform the same implementation.
--
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] pan3793 commented on a change in pull request #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805498034
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/PlanOnlyStatement.scala
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.flink.operation
+
+import org.apache.flink.table.api.TableEnvironment
+import org.apache.flink.table.operations.command.{ResetOperation, SetOperation}
+
+import org.apache.kyuubi.KyuubiSQLException
+import org.apache.kyuubi.config.KyuubiConf.OperationModes._
+import org.apache.kyuubi.engine.flink.result.ResultSetUtil
+import org.apache.kyuubi.operation.OperationType
+import org.apache.kyuubi.operation.log.OperationLog
+import org.apache.kyuubi.session.Session
+
+/**
+ * Perform the statement parsing, analyzing or optimizing only without executing it
+ */
+class PlanOnlyStatement(
+ session: Session,
+ override val statement: String,
+ mode: OperationMode)
+ extends FlinkOperation(OperationType.EXECUTE_STATEMENT, session) {
+
+ private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
+ override def getOperationLog: Option[OperationLog] = Option(operationLog)
+
+ override protected def runInternal(): Unit = {
+ try {
+ val operation = executor.parseStatement(sessionId, statement)
+ operation match {
+ case setOperation: SetOperation =>
+ resultSet = ResultSetUtil.runSetOperation(setOperation, executor, sessionId)
+ case resetOperation: ResetOperation =>
+ resultSet = ResultSetUtil.runResetOperation(resetOperation, executor, sessionId)
+ case _ => explainOperation(statement)
+ }
+ } catch {
+ onError()
+ }
+ }
+
+ private def explainOperation(statement: String): Unit = {
+ val tableEnv: TableEnvironment = sessionContext.getExecutionContext.getTableEnvironment
+ mode match {
+ case PARSE =>
+ val sqlPlan = tableEnv.explainSql(statement)
+ resultSet =
+ ResultSetUtil.stringListToResultSet(
+ List(sqlPlan.split(System.lineSeparator()).apply(1)),
+ "plan")
+ case _ =>
+ throw KyuubiSQLException(
+ s"The operation mode ${mode.toString} doesn't support in Flink SQL engine.")
Review comment:
Would you please give some context about Flink plan mode? i.e., which phase does Flink support at now or in future?
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038591285
@yaooqinn @yanghua, thanks for your review. I have addressed above comments. Please help to review again.
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas removed a comment on pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#issuecomment-1038659169
@pan3793, thanks for your detailed review. I have addressed above comments. Please help to review again.
--
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 #1891: [KYUUBI #1867] Support `PlanOnlyStatement` operation like Spark SQL engine
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on a change in pull request #1891:
URL: https://github.com/apache/incubator-kyuubi/pull/1891#discussion_r805595434
##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
##########
@@ -107,10 +105,13 @@ class ExecuteStatement(
val operation = executor.parseStatement(sessionId, statement)
operation match {
case queryOperation: QueryOperation => runQueryOperation(queryOperation)
- case setOperation: SetOperation => runSetOperation(setOperation)
- case resetOperation: ResetOperation => runResetOperation(resetOperation)
+ case setOperation: SetOperation =>
Review comment:
@yanghua, I don't agree with you. If the mode is `NONE`, the `SetOperation` and `ResetOperation` couldn't be executed.
--
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