Skip to content

Conversation

@cfmcgrady
Copy link
Contributor

@cfmcgrady cfmcgrady commented Dec 20, 2021

Why are the changes needed?

  1. move spark-3.1 ForcedMaxOutputRowsRule to spark-common and rename to ForcedMaxOutputRowsBase
  2. handle WithCTE logical plan in spark-3.2
  3. move spark-3.1 MaxPartitionStrategy to spark-common
  4. add netsted cte unit test for ForcedMaxOutputRowsRule

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

@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2021

Codecov Report

Merging #1591 (5399a3f) into master (df1d9f3) will decrease coverage by 0.95%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1591      +/-   ##
============================================
- Coverage     59.02%   58.06%   -0.96%     
+ Complexity      196      140      -56     
============================================
  Files           256      256              
  Lines         12708    12683      -25     
  Branches       1601     1596       -5     
============================================
- Hits           7501     7365     -136     
- Misses         4570     4695     +125     
+ Partials        637      623      -14     
Impacted Files Coverage Δ
.../kyuubi/sql/watchdog/ForcedMaxOutputRowsBase.scala 0.00% <0.00%> (ø)
.../kyuubi/sql/watchdog/KyuubiWatchDogException.scala 0.00% <ø> (ø)
...che/kyuubi/sql/watchdog/MaxPartitionStrategy.scala 0.00% <ø> (ø)
...che/spark/sql/PruneFileSourcePartitionHelper.scala 0.00% <ø> (ø)
...he/kyuubi/engine/spark/repl/KyuubiSparkILoop.scala 90.16% <0.00%> (-3.28%) ⬇️
.../kyuubi/sql/watchdog/ForcedMaxOutputRowsRule.scala

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df1d9f3...5399a3f. Read the comment docs.

@cfmcgrady
Copy link
Contributor Author

cc @ulysses-you and watchdog original author @zhouyifan279 @i7xh

|SELECT * FROM t2
|$sort
|LIMIT $limit
|""".stripMargin).queryExecution.optimizedPlan.maxRows.contains(expected))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As WithCTE.maxRows is None, we should check optimizedPlan here.

|$having
|$sort
|LIMIT $limit
|""".stripMargin).queryExecution.optimizedPlan.maxRows.contains(expected))
Copy link
Contributor Author

Choose a reason for hiding this comment

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


}

trait MarkAggregateOrderBase extends Rule[LogicalPlan] {
Copy link
Contributor

Choose a reason for hiding this comment

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

For Spark 3.2, we don't need MarkAggregateOrderBase about aggregate since apache/spark#32470. We only need CTE.

@ulysses-you
Copy link
Contributor

thanks, merging to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants