LuciferYang commented on code in PR #43745:
URL: https://github.com/apache/spark/pull/43745#discussion_r1389784279
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -250,15 +250,18 @@ class SparkSession private[sql] (
.setSql(sqlText)
.addAllPosArguments(args.map(lit(_).expr).toImmutableArraySeq.asJava)))
val plan = proto.Plan.newBuilder().setCommand(cmd)
- // .toBuffer forces that the iterator is consumed and closed
- val responseSeq = client.execute(plan.build()).toBuffer.toSeq
+ val responseIter = client.execute(plan.build())
- val response = responseSeq
- .find(_.hasSqlCommandResult)
- .getOrElse(throw new RuntimeException("SQLCommandResult must be
present"))
-
- // Update the builder with the values from the result.
- builder.mergeFrom(response.getSqlCommandResult.getRelation)
+ try {
+ val response = responseIter
Review Comment:
I have changed the two relevant places to `find+foreach`
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -543,14 +549,14 @@ class SparkSession private[sql] (
f(builder)
builder.getCommonBuilder.setPlanId(planIdGenerator.getAndIncrement())
val plan = proto.Plan.newBuilder().setRoot(builder).build()
- // .toBuffer forces that the iterator is consumed and closed
- client.execute(plan).toBuffer
+ // .foreach forces that the iterator is consumed and closed
+ client.execute(plan).foreach(_ => ())
}
private[sql] def execute(command: proto.Command): Seq[ExecutePlanResponse] =
{
val plan = proto.Plan.newBuilder().setCommand(command).build()
- // .toBuffer forces that the iterator is consumed and closed
- client.execute(plan).toBuffer.toSeq
+ // .toSeq forces that the iterator is consumed and closed
+ client.execute(plan).toSeq
Review Comment:
I think the new test is still useful because only `.toSeq` can be used here`
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -543,14 +549,14 @@ class SparkSession private[sql] (
f(builder)
builder.getCommonBuilder.setPlanId(planIdGenerator.getAndIncrement())
val plan = proto.Plan.newBuilder().setRoot(builder).build()
- // .toBuffer forces that the iterator is consumed and closed
- client.execute(plan).toBuffer
+ // .foreach forces that the iterator is consumed and closed
+ client.execute(plan).foreach(_ => ())
Review Comment:
Because the return type is `Unit`, foreach should also be used here, right?
--
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]
