[KYUUBI #3214] Plan only mode should unset when mode value is incorrect

[lsm1]

Why are the changes needed?

close #3214

manual tests

when we set kyuubi.operation.plan.only.mode an incorrect value,unset plan only mode so we can set a correct value

How was this patch tested?

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

• [x] Add screenshots for manual tests if appropriate

• [ ] Run test locally before make a pull request

[codecov-commenter]

Codecov Report

Merging #3216 (bb159ec) into master (8c2e774) will decrease coverage by 0.01%. The diff coverage is 52.94%.

@@             Coverage Diff              @@
##             master    #3216      +/-   ##
============================================
- Coverage     50.94%   50.92%   -0.02%     
  Complexity       13       13              
============================================
  Files           469      469              
  Lines         26123    26136      +13     
  Branches       3612     3619       +7     
============================================
+ Hits          13308    13310       +2     
- Misses        11544    11549       +5     
- Partials       1271     1277       +6     

Impacted Files	Coverage Δ
...ubi/engine/spark/operation/PlanOnlyStatement.scala	62.22% <0.00%> (-4.45%)	:arrow_down:
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala	96.86% <54.54%> (-0.49%)	:arrow_down:
...ine/spark/operation/SparkSQLOperationManager.scala	83.60% <100.00%> (+0.27%)	:arrow_up:
...ain/scala/org/apache/kyuubi/engine/EngineRef.scala	74.07% <0.00%> (-0.93%)	:arrow_down:
...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala	67.50% <0.00%> (-0.63%)	:arrow_down:
...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala	82.60% <0.00%> (-0.63%)	:arrow_down:
...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala	80.00% <0.00%> (+2.00%)	:arrow_up:

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

[cxzl25]

FlinkSQLOperationManager has a similar problem.

We can add a UT in PlanOnlyOperationSuite.

[wForget]

unset conf doesn't seem to be a good way:

set kyuubi.operation.plan.only.mode=parser; // success
select 1; // exception
select 1; // success without plan mode

we can add UNKNOWN type to OperationModes and throw an exception in PlanOnlyStatement#runInternal.

[yaooqinn]

I suggest

• we implement it in OperationModes for reuse in different engines.
• redirect unrecognized modes to NONE with a warning message, it has a deterministic behavior which does not rely on kyuubi.operation.plan.only.excludes

  object OperationModes extends Enumeration with Logging {
    type OperationMode = Value
    val PARSE, ANALYZE, OPTIMIZE, PHYSICAL, EXECUTION, NONE = Value

    def apply(mode: String): OperationMode = {
      mode.toUpperCase(Locale.ROOT) match {
        case "PARSE" => PARSE
        case "ANALYZE" => ANALYZE
        case "OPTIMIZE" => OPTIMIZE
        case "PHYSICAL" => PHYSICAL
        case "EXECUTION" => EXECUTION
        case "NONE" => NONE
        case other =>
          warn(s"Unsupported operation mode: $mode, using NONE instead")
          NONE
      }
    }
  }

Accordingly in OperationManagers,

       case OperationLanguages.SQL =>
          val mode =
            OperationModes(spark.conf.get(OPERATION_PLAN_ONLY_MODE.key, operationModeDefault))
          mode match {
            case NONE =>
              val incrementalCollect = spark.conf.getOption(OPERATION_INCREMENTAL_COLLECT.key)
                .map(_.toBoolean).getOrElse(operationIncrementalCollectDefault)
              new ExecuteStatement(session, statement, runAsync, queryTimeout, incrementalCollect)
            case mode =>
              new PlanOnlyStatement(session, statement, mode)
          }

[wForget]

redirect unrecognized modes to NONE with a warning message, it has a deterministic behavior which does not rely on kyuubi.operation.plan.only.excludes

When we set a wrong plan mode, there is no any exception, then we execute the insert command successfully, which seems dangerous.

this test works because of kyuubi.operation.plan.only.excludes. So this is actually a side-effect and indeterministic.

It looks like this, but if we need to switch plan mode normally, we also rely on kyuubi.operation.plan.only.excludes:

set kyuubi.operation.plan.only.mode=parse;
set kyuubi.operation.plan.only.mode=none;

[yaooqinn]

make sense, let's add an unknown type here

[yaooqinn]

Modified a bit according to @wForget 's comments

I suggest

• we implement it in OperationModes for reuse in different engines.
• redirect unrecognized modes to NONE with a warning message, it has a deterministic behavior which does not rely on kyuubi.operation.plan.only.excludes

  object OperationModes extends Enumeration with Logging {
    type OperationMode = Value
    val PARSE, ANALYZE, OPTIMIZE, PHYSICAL, EXECUTION, NONE, UNKNOWN = Value

    def apply(mode: String): OperationMode = {
      mode.toUpperCase(Locale.ROOT) match {
        case "PARSE" => PARSE
        case "ANALYZE" => ANALYZE
        case "OPTIMIZE" => OPTIMIZE
        case "PHYSICAL" => PHYSICAL
        case "EXECUTION" => EXECUTION
        case "NONE" => NONE
        case other =>
          warn(s"Unsupported operation mode: $other, using UNKNOWN instead")
          UNKNOWN
      }
    }
  }

Accordingly in OperationManagers,

       case OperationLanguages.SQL =>
          val mode =
            OperationModes(spark.conf.get(OPERATION_PLAN_ONLY_MODE.key, operationModeDefault))
          spark.conf.set(OPERATION_PLAN_ONLY_MODE.key, mode.toString)
          mode match {
            case NONE =>
              val incrementalCollect = spark.conf.getOption(OPERATION_INCREMENTAL_COLLECT.key)
                .map(_.toBoolean).getOrElse(operationIncrementalCollectDefault)
              new ExecuteStatement(session, statement, runAsync, queryTimeout, incrementalCollect)
            case mode =>
              new PlanOnlyStatement(session, statement, mode)
          }

[yaooqinn]

LGTM

[pan3793]

Thanks, merging to master