juliuszsompolski commented on code in PR #42454:
URL: https://github.com/apache/spark/pull/42454#discussion_r1294729499
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/SparkConnectPlanExecution.scala:
##########
@@ -163,7 +165,6 @@ private[execution] class
SparkConnectPlanExecution(executeHolder: ExecuteHolder)
// Collect errors and propagate them to the main thread.
.andThen {
case Success(_) =>
- executePlan.eventsManager.postFinished()
Review Comment:
Is the empty case Success still needed to avoid an unexhaustive match, or
rather is andThen taking a PartialFunction and just skipping case Success is
fine? If it is not needed I'd remove; if it's needed, I'd add a comment like
`// do nothing` instead of leaving it dangling like that.
(should be easy to test - if you remove it, and it expected it to be there,
it should crash and burn everywhere, as this is the common code path; or it
will work and it means it's fine)
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecuteEventsManager.scala:
##########
@@ -397,11 +416,15 @@ case class SparkListenerConnectOperationFailed(
* The time in ms when the event was generated.
* @param extraTags:
* Additional metadata during the request.
+ * @param producedRowCount:
+ * Number of rows that are returned to the user. None is expected when the
operation does not
+ * return any rows.
Review Comment:
very nit: order before extraTags, like the order of params?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
