[SPARK-44776][CONNECT] Add ProducedRowCount to SparkListenerConnectOperationFinished #42454
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdesjean
reviewed
Aug 11, 2023
...server/src/main/scala/org/apache/spark/sql/connect/execution/SparkConnectPlanExecution.scala
Show resolved
Hide resolved
HyukjinKwon
changed the title
jdesjean
approved these changes
Aug 15, 2023
...onnect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecuteEventsManager.scala
Outdated
Show resolved
Hide resolved
...ct/server/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectServiceSuite.scala
Show resolved
Hide resolved
jdesjean
reviewed
Aug 15, 2023
...server/src/main/scala/org/apache/spark/sql/connect/execution/SparkConnectPlanExecution.scala
Outdated
Show resolved
Hide resolved
Contributor
juliuszsompolski
left a comment
juliuszsompolski
left a comment
There was a problem hiding this comment.
Run
./build/mvn -Pscala-2.12 scalafmt:format -Dscalafmt.skip=false -Dscalafmt.validateOnly=false -Dscalafmt.changedOnly=false -pl connector/connect/common -pl connector/connect/server -pl connector/connect/client/jvm
to lint
...server/src/main/scala/org/apache/spark/sql/connect/execution/SparkConnectPlanExecution.scala
Outdated
Show resolved
Hide resolved
...onnect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecuteEventsManager.scala
Show resolved
Hide resolved
juliuszsompolski
approved these changes
Aug 15, 2023
...server/src/main/scala/org/apache/spark/sql/connect/execution/SparkConnectPlanExecution.scala
Show resolved
Hide resolved
...onnect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecuteEventsManager.scala
Show resolved
Hide resolved
| /** | ||
| * Post @link org.apache.spark.sql.connect.service.SparkListenerConnectOperationFinished. | ||
| * @param producedRowsCountOpt | ||
| * Number of rows that are returned to the user. None is expected when the operation does not |
Member
There was a problem hiding this comment.
QQ: why not use 0 if not rows are returned?
Author
There was a problem hiding this comment.
I think returning None would make it clear to us whether the corresponding query should have produced row or not, for example for a SELECT statement that has no result, we would put 0 here. And for query Like INSERT INTO, we would put a None instead of 0 here. This could better tell us whether a produced row is expected or not
Contributor
There was a problem hiding this comment.
Agreed with gjxdxh that it's clearer with None that no rows are expected for this operation.
HyukjinKwon
approved these changes
Aug 21, 2023
Member
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
LGTM but I would like @gengliangwang 's comment to be addressed before merging it :-).
Member
|
Merged to master and branch-3.5. |
HyukjinKwon
pushed a commit
that referenced
this pull request
Aug 22, 2023
…erationFinished ### What changes were proposed in this pull request? Add ProducedRowCount field to SparkListenerConnectOperationFinished ### Why are the changes needed? Needed for showing number of rows getting produced ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added Unit test Closes #42454 from gjxdxh/SPARK-44776. Authored-by: Lingkai Kong <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]> (cherry picked from commit 4646991) Signed-off-by: Hyukjin Kwon <[email protected]>
valentinp17
pushed a commit
to valentinp17/spark
that referenced
this pull request
Aug 24, 2023
…erationFinished ### What changes were proposed in this pull request? Add ProducedRowCount field to SparkListenerConnectOperationFinished ### Why are the changes needed? Needed for showing number of rows getting produced ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added Unit test Closes apache#42454 from gjxdxh/SPARK-44776. Authored-by: Lingkai Kong <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
This was referenced Sep 12, 2023
dongjoon-hyun
added a commit
that referenced
this pull request
May 7, 2024
…-43923: commands send events - get_resources_command` ### What changes were proposed in this pull request? This PR aims to disable a flaky test, `SparkConnectServiceSuite.SPARK-43923: commands send events - get_resources_command`, temporarily. To re-enable this, SPARK-48164 is created as a blocker issue for 4.0.0. ### Why are the changes needed? This test case was added at `Apache Spark 3.5.0`, but it has been flaky and causes many re-tries in our GitHub Action CI environment. - #42454 - https://github.com/apache/spark/actions/runs/8979348499/job/24661200052 ``` [info] - SPARK-43923: commands send events ((get_resources_command { [info] } [info] ,None)) *** FAILED *** (35 milliseconds) [info] VerifyEvents.this.listener.executeHolder.isDefined was false (SparkConnectServiceSuite.scala:873) ``` This PR aims to stabilize CI first and to focus this flaky issue as a blocker level before going on `Spark Connect GA` in SPARK-48164 before Apache Spark 4.0.0. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46425 from dongjoon-hyun/SPARK-48163. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Add ProducedRowCount field to SparkListenerConnectOperationFinished
Why are the changes needed?
Needed for showing number of rows getting produced
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added Unit test