Skip to content

Conversation

@lsm1
Copy link
Contributor

@lsm1 lsm1 commented Sep 7, 2022

Why are the changes needed?

close #3435

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@wForget
Copy link
Member

wForget commented Sep 9, 2022

This change doesn't seem to be compatible with Spark 3.1, can you make it compatible?

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

Merging #3439 (3447cbf) into master (b418f89) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##             master    #3439   +/-   ##
=========================================
  Coverage     51.92%   51.92%           
  Complexity       13       13           
=========================================
  Files           508      522   +14     
  Lines         29029    29049   +20     
  Branches       3985     3887   -98     
=========================================
+ Hits          15072    15084   +12     
- Misses        12532    12590   +58     
+ Partials       1425     1375   -50     
Impacted Files Coverage Δ
...ubi/engine/spark/operation/PlanOnlyStatement.scala 39.78% <0.00%> (-3.24%) ⬇️
...uubi/plugin/spark/authz/ranger/AccessRequest.scala 71.42% <0.00%> (-22.33%) ⬇️
...mon/src/main/scala/org/apache/kyuubi/Logging.scala 43.42% <0.00%> (-9.22%) ⬇️
.../main/scala/org/apache/kyuubi/util/SignUtils.scala 87.87% <0.00%> (-8.68%) ⬇️
.../kyuubi/operation/KyuubiApplicationOperation.scala 40.90% <0.00%> (-6.46%) ⬇️
.../kyuubi/server/mysql/constant/MySQLErrorCode.scala 13.84% <0.00%> (-6.16%) ⬇️
...pache/kyuubi/engine/YarnApplicationOperation.scala 58.73% <0.00%> (-4.24%) ⬇️
...apache/kyuubi/session/KyuubiBatchSessionImpl.scala 86.58% <0.00%> (-4.09%) ⬇️
...ache/kyuubi/server/mysql/MySQLCommandHandler.scala 77.77% <0.00%> (-4.05%) ⬇️
...uubi/plugin/spark/authz/util/SemanticVersion.scala 50.00% <0.00%> (-3.58%) ⬇️
... and 68 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

val physical = spark.sessionState.planner.plan(ReturnAnswer(optimized)).next()
iter = new IterableFetchIterator(Seq(Row(physical.toJSON)))
case ExecutionMode =>
warn("Plan like Command will real execute")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExplainCommand return string, when use json style ,the plan cannot be wrapped as ExplainCommand.Add warning for this case

@yaooqinn
Copy link
Member

shall we remove this two? they are dangerous and not very useful

@lsm1
Copy link
Contributor Author

lsm1 commented Sep 13, 2022

SPARK-35378 add assertCommandExecuted since spark 3.2.0,spark 3.1 is normal. we can fix this case except execution mode in json style, can we just remove execution mode in json style?

@yaooqinn
Copy link
Member

ok

@yaooqinn
Copy link
Member

this introduces unstable private class from spark, not a good solution to me

@lsm1 lsm1 force-pushed the features/optimizer_only_plan_mode branch from 6c49c3b to 9aabe04 Compare December 12, 2022 03:25
@cxzl25
Copy link
Contributor

cxzl25 commented Aug 12, 2024

#6575

@cxzl25 cxzl25 closed this Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Command should not execute when plan only mode is set to PHYSICAL or EXECUTION

5 participants