Fix DataWritingCommand callback naver be called on withListener method#7362
Fix DataWritingCommand callback naver be called on withListener method#7362wForget wants to merge 1 commit intoapache:masterfrom
Conversation
|
Duplicate #6793 |
| override def onSuccess(funcName: String, qe: QueryExecution, duration: Long): Unit = { | ||
| qe.executedPlan match { | ||
| case write: DataWritingCommandExec => callback(write.cmd) | ||
| collect(qe.executedPlan) { |
There was a problem hiding this comment.
DataWritingCommandExec is wrapped by AdaptiveSparkPlanExec, so we should use AdaptiveSparkPlanHelper#collect
There was a problem hiding this comment.
Pull request overview
This PR fixes a test utility (withListener) used by Spark SQL extension test suites to reliably observe DataWritingCommandExec by traversing the executed physical plan, and adds an option to allow queries that legitimately have no write command.
Changes:
- Update
withListenerto search the whole executed plan forDataWritingCommandExecand track whether the callback ran. - Add a
mustBeCalledflag (defaulttrue) to assert callback execution when expected. - Update one rebalance-related test to opt out of the callback-must-run assertion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
extensions/spark/kyuubi-extension-spark-4-1/src/test/scala/org/apache/spark/sql/KyuubiSparkSQLExtensionTest.scala |
Makes withListener robust by scanning the executed plan and asserting callback execution by default. |
extensions/spark/kyuubi-extension-spark-4-1/src/test/scala/org/apache/spark/sql/RebalanceBeforeWritingSuite.scala |
Uses the new mustBeCalled=false mode in a test that includes non-write queries. |
Comments suppressed due to low confidence (1)
extensions/spark/kyuubi-extension-spark-4-1/src/test/scala/org/apache/spark/sql/RebalanceBeforeWritingSuite.scala:116
- Setting
mustBeCalled = falseinside the sharedcheckhelper means this test will still pass even if the listener never finds aDataWritingCommandExecfor INSERT queries (the callback won’t run, so the rebalance assertion won’t execute). To avoid masking regressions, consider usingmustBeCalled=falseonly for the SELECT/COUNT cases that intentionally have no write command, and keep the defaultmustBeCalled=truefor INSERT statements.
test("check rebalance does not exists") {
def check(df: DataFrame): Unit = {
withListener(df, false) { write =>
assert(write.collect {
case r: RebalancePartitions => r
}.isEmpty)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| case write: DataWritingCommandExec => | ||
| called.set(true) | ||
| callback(write.cmd) | ||
| case _ => |
Why are the changes needed?
The DataWritingCommand callback naver be called on withListener method
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No