You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "szehon-ho (via GitHub)" <gi...@apache.org> on 2025/02/20 23:58:38 UTC
[PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
szehon-ho opened a new pull request, #50031:
URL: https://github.com/apache/spark/pull/50031
### What changes were proposed in this pull request?
Return CommandResult in InvokeProcedure rule instead of LocalRelation
### Why are the changes needed?
Running 'call procedure' via Spark connect results in the procedure getting called twice.
This is because the `org.apache.spark.sql.connect.SparkSession.sql` method sends the plan over to be evaluated, and that invokes it once.
The plan is embedded a org.apache.spark.sql.connect.DataSet, and then running df.collect() sends the plan to be evaluated, invoking it a second time.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added a SparkConnect integration test using Procedure Catalog
### Was this patch authored or co-authored using generative AI tooling?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1970429680
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
Yea I took a quick look when making this test run on both environments, it seems createProcedure (used in ProcedureSuite) is harder to do in the connect test environment as direct access to the catalog is currently not possible. We could maybe investigate it further in subsequent ticket?
There's only one example that I know , that @cloud-fan pointed out in https://github.com/apache/spark/pull/50031#discussion_r1967751946 Other difficulties is , QueryTest.checkResult which is quite popular, is actually different classes in connect and sql test to avoid adding test dependencies in connect.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1967645049
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryProcedureCatalog.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector.catalog
+
+import java.util
+import java.util.Collections
+import java.util.concurrent.atomic.AtomicInteger
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.connector.catalog.procedures.{BoundProcedure, ProcedureParameter, UnboundProcedure}
+import org.apache.spark.sql.connector.read.{LocalScan, Scan}
+import org.apache.spark.sql.types.{DataTypes, StructType}
+
+
+class InMemoryProcedureCatalog extends InMemoryTableCatalog
+ with ProcedureCatalog {
Review Comment:
OK we need a single catalog for both table and procedure testing. Then shall we simply put this procedure implementation in `InMemoryTableCatalog`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1967642091
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryProcedureCatalog.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector.catalog
+
+import java.util
+import java.util.Collections
+import java.util.concurrent.atomic.AtomicInteger
+
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.connector.catalog.procedures.{BoundProcedure, ProcedureParameter, UnboundProcedure}
+import org.apache.spark.sql.connector.read.{LocalScan, Scan}
+import org.apache.spark.sql.types.{DataTypes, StructType}
+
+
+class InMemoryProcedureCatalog extends InMemoryTableCatalog
+ with ProcedureCatalog {
Review Comment:
Can we simply do `class InMemoryProcedureCatalog extends ProcedureCatalog`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #50031:
URL: https://github.com/apache/spark/pull/50031#issuecomment-2675569385
@aokolnychyi @cloud-fan do you guys want to take a look, thanks!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1970224742
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/InvokeProcedures.scala:
##########
@@ -36,20 +37,35 @@ class InvokeProcedures(session: SparkSession) extends Rule[LogicalPlan] {
case c: Call if c.resolved && c.bound && c.execute && c.checkArgTypes().isSuccess =>
session.sessionState.optimizer.execute(c) match {
case Call(ResolvedProcedure(_, _, procedure: BoundProcedure), args, _) =>
- invoke(procedure, args)
+ invoke(procedure, args, c)
case _ =>
throw SparkException.internalError("Unexpected plan for optimized CALL statement")
}
}
- private def invoke(procedure: BoundProcedure, args: Seq[Expression]): LogicalPlan = {
+ private def invoke(procedure: BoundProcedure, args: Seq[Expression],
+ callCommand: Call): LogicalPlan = {
Review Comment:
What if we simply call it `call` so that the statement fits on one line?
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
How hard will it be to make `ProcedureSuite` generic so that we can verify all tests in connect instead of just checking one? Are there existing examples of such suites? What's our approach when the same logic must be checked across classic and connect?
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/InvokeProcedures.scala:
##########
@@ -36,20 +37,35 @@ class InvokeProcedures(session: SparkSession) extends Rule[LogicalPlan] {
case c: Call if c.resolved && c.bound && c.execute && c.checkArgTypes().isSuccess =>
session.sessionState.optimizer.execute(c) match {
case Call(ResolvedProcedure(_, _, procedure: BoundProcedure), args, _) =>
- invoke(procedure, args)
+ invoke(procedure, args, c)
case _ =>
throw SparkException.internalError("Unexpected plan for optimized CALL statement")
}
}
- private def invoke(procedure: BoundProcedure, args: Seq[Expression]): LogicalPlan = {
+ private def invoke(procedure: BoundProcedure, args: Seq[Expression],
+ callCommand: Call): LogicalPlan = {
val input = toInternalRow(args)
val scanIterator = procedure.call(input)
val relations = scanIterator.asScala.map(toRelation).toSeq
+ // Wrap the result in a CommandResult so SparkConnect
+ // recognizes it as an eagerly executed SQL Command.
relations match {
- case Nil => LocalRelation(Nil)
- case Seq(relation) => relation
- case _ => MultiResult(relations)
+ case Nil =>
+ CommandResult(Seq.empty,
+ callCommand,
+ LocalTableScanExec(Seq.empty, Seq.empty, None),
+ Seq.empty)
+ case Seq(relation: LocalRelation) =>
+ CommandResult(
+ relation.output,
+ callCommand,
+ LocalTableScanExec(relation.output, relation.data, None),
+ relation.data)
+ case _ =>
+ // TODO wrap in command result to support SparkConnect
Review Comment:
I am not sure there is value in this TODO. It is not clear that `CommandResult` would be the final solution and it must be extended to support multiple results. We may make `MultiResult` work in connect instead.
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/InvokeProcedures.scala:
##########
@@ -36,20 +37,35 @@ class InvokeProcedures(session: SparkSession) extends Rule[LogicalPlan] {
case c: Call if c.resolved && c.bound && c.execute && c.checkArgTypes().isSuccess =>
session.sessionState.optimizer.execute(c) match {
case Call(ResolvedProcedure(_, _, procedure: BoundProcedure), args, _) =>
- invoke(procedure, args)
+ invoke(procedure, args, c)
case _ =>
throw SparkException.internalError("Unexpected plan for optimized CALL statement")
}
}
- private def invoke(procedure: BoundProcedure, args: Seq[Expression]): LogicalPlan = {
+ private def invoke(procedure: BoundProcedure, args: Seq[Expression],
+ callCommand: Call): LogicalPlan = {
val input = toInternalRow(args)
val scanIterator = procedure.call(input)
val relations = scanIterator.asScala.map(toRelation).toSeq
+ // Wrap the result in a CommandResult so SparkConnect
+ // recognizes it as an eagerly executed SQL Command.
relations match {
- case Nil => LocalRelation(Nil)
- case Seq(relation) => relation
- case _ => MultiResult(relations)
+ case Nil =>
+ CommandResult(Seq.empty,
+ callCommand,
+ LocalTableScanExec(Seq.empty, Seq.empty, None),
+ Seq.empty)
+ case Seq(relation: LocalRelation) =>
+ CommandResult(
+ relation.output,
+ callCommand,
+ LocalTableScanExec(relation.output, relation.data, None),
+ relation.data)
+ case _ =>
+ // TODO wrap in command result to support SparkConnect
+ throw SparkException.internalError(
+ s"Multi-result procedures are temporarily not supported.")
Review Comment:
Do we need `.` at the end? Other error messages in this class don't use it. The statement may actually fit on one line without it.
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/InvokeProcedures.scala:
##########
@@ -36,20 +37,35 @@ class InvokeProcedures(session: SparkSession) extends Rule[LogicalPlan] {
case c: Call if c.resolved && c.bound && c.execute && c.checkArgTypes().isSuccess =>
session.sessionState.optimizer.execute(c) match {
case Call(ResolvedProcedure(_, _, procedure: BoundProcedure), args, _) =>
- invoke(procedure, args)
+ invoke(procedure, args, c)
case _ =>
throw SparkException.internalError("Unexpected plan for optimized CALL statement")
}
}
- private def invoke(procedure: BoundProcedure, args: Seq[Expression]): LogicalPlan = {
+ private def invoke(procedure: BoundProcedure, args: Seq[Expression],
+ callCommand: Call): LogicalPlan = {
val input = toInternalRow(args)
val scanIterator = procedure.call(input)
val relations = scanIterator.asScala.map(toRelation).toSeq
+ // Wrap the result in a CommandResult so SparkConnect
+ // recognizes it as an eagerly executed SQL Command.
relations match {
- case Nil => LocalRelation(Nil)
- case Seq(relation) => relation
- case _ => MultiResult(relations)
+ case Nil =>
+ CommandResult(Seq.empty,
Review Comment:
Minor: Shall we move `Seq.empty` to a new line for consistency?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1967751946
##########
sql/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/ProcedureSuite.scala:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connect
+
+import org.scalatest.BeforeAndAfter
+
+import org.apache.spark.sql.Row
+import org.apache.spark.sql.connect.test.{QueryTest, RemoteSparkSession}
+
+class ProcedureSuite extends QueryTest with RemoteSparkSession with BeforeAndAfter {
Review Comment:
can we follow `SparkSessionBuilderImplementationBindingSuite` to define the suite for both classic and connect?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1971589160
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
I think it's a big blocker for writing unified tests between classic and connect. cc @hvanhovell
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1970450302
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
Yeah, it does seem it would be rather complicated. If so, I am OK as is.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #50031:
URL: https://github.com/apache/spark/pull/50031#issuecomment-2686011498
Also wanted to give credit to @raveeram-db to identify the issue and the idea for the fix!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #50031: [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice
URL: https://github.com/apache/spark/pull/50031
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1971823853
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
@cloud-fan we want to do the test unification in Spark 4. One of the more important tasks there is to come up with a unified testing harness, and some infrastructure to make it easy to run tests in both modes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50031:
URL: https://github.com/apache/spark/pull/50031#issuecomment-2686523433
thanks, merging to master/4.0!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1970433784
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
There's only one example that I know , that @cloud-fan pointed out in #50031 (comment) Other difficulties is , QueryTest.checkResult which is quite popular, is actually different classes in connect and sql test to avoid adding test dependencies in connect.
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
Yea I took a quick look when making this test run on both environments, it seems createProcedure (used in ProcedureSuite) is harder to do in the connect test environment as direct access to the catalog is currently not possible. We could maybe investigate it further in subsequent ticket?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1970433784
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
There's only one example that I know , that @cloud-fan pointed out in [(comment)](https://github.com/apache/spark/pull/50031#discussion_r1967751946) Other difficulties is , QueryTest.checkResult which is quite popular, is actually different classes in connect and sql test to avoid adding test dependencies in connect.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1972592710
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
That would be awesome!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1970429680
##########
sql/api/src/test/scala/org/apache/spark/sql/StaticProcedureSuiteBase.scala:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+// scalastyle:off funsuite
+import org.scalatest.funsuite.AnyFunSuite
+
+trait StaticProcedureSuiteBase extends AnyFunSuite {
Review Comment:
Yea I took a quick look when making this test run on both environments, it seems createProcedure (used in ProcedureSuite) is harder to do in the connect test environment as direct access to the catalog is currently not possible. We could maybe investigate it further in subsequent ticket?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51273][SQL] Spark Connect Call Procedure runs the procedure twice [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50031:
URL: https://github.com/apache/spark/pull/50031#discussion_r1970445327
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/InvokeProcedures.scala:
##########
@@ -36,20 +37,35 @@ class InvokeProcedures(session: SparkSession) extends Rule[LogicalPlan] {
case c: Call if c.resolved && c.bound && c.execute && c.checkArgTypes().isSuccess =>
session.sessionState.optimizer.execute(c) match {
case Call(ResolvedProcedure(_, _, procedure: BoundProcedure), args, _) =>
- invoke(procedure, args)
+ invoke(procedure, args, c)
case _ =>
throw SparkException.internalError("Unexpected plan for optimized CALL statement")
}
}
- private def invoke(procedure: BoundProcedure, args: Seq[Expression]): LogicalPlan = {
+ private def invoke(procedure: BoundProcedure, args: Seq[Expression],
+ callCommand: Call): LogicalPlan = {
val input = toInternalRow(args)
val scanIterator = procedure.call(input)
val relations = scanIterator.asScala.map(toRelation).toSeq
+ // Wrap the result in a CommandResult so SparkConnect
Review Comment:
It seems reasonable to switch to `CommandResult` for now. We will have to adapt `SparkConnectPlanner` eventually to support `MultiResult`. No need to look into that now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org