You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dillitz (via GitHub)" <gi...@apache.org> on 2023/10/10 12:42:21 UTC
[PR] [SPARK-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner [spark]
dillitz opened a new pull request, #43311:
URL: https://github.com/apache/spark/pull/43311
### What changes were proposed in this pull request?
This PR adds an optional `ExecuteHolder` to `SparkConnectPlanner`.
This allows plugins to access the `ExecuteHolder` to for example create a `QueryPlanningTracker` without the need to create a new `CommandPlugin` interface like proposed in https://github.com/apache/spark/pull/42984.
### Why are the changes needed?
There is currently no way to track queries executed by a CommandPlugin. For this, the plugin needs access to the `ExecuteHolder`.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Adjusted existing tests.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #43311:
URL: https://github.com/apache/spark/pull/43311#discussion_r1355321391
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -86,14 +86,29 @@ final case class InvalidCommandInput(
private val cause: Throwable = null)
extends Exception(message, cause)
-class SparkConnectPlanner(val sessionHolder: SessionHolder) extends Logging {
+class SparkConnectPlanner(
+ val sessionHolder: SessionHolder,
+ val executeHolderOpt: Option[ExecuteHolder] = None)
Review Comment:
it's used in AnalyzePlan that doesn't have an ExecuteHolder
--
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-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #43311:
URL: https://github.com/apache/spark/pull/43311#discussion_r1352598429
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -2461,48 +2475,39 @@ class SparkConnectPlanner(val sessionHolder: SessionHolder) extends Logging {
def process(
command: proto.Command,
- responseObserver: StreamObserver[ExecutePlanResponse],
- executeHolder: ExecuteHolder): Unit = {
+ responseObserver: StreamObserver[ExecutePlanResponse]): Unit = {
Review Comment:
Conceptually, this shouldn't be an issue because it's not really a public API. but it's not backwards compatible.
This is particularly interesting because you went through some length to fix this problem in the constructor though.
@hvanhovell Do we consider the SparkConenct planner to be public? Probably, not correct?
--
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-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #43311:
URL: https://github.com/apache/spark/pull/43311#discussion_r1353107388
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -2461,48 +2475,39 @@ class SparkConnectPlanner(val sessionHolder: SessionHolder) extends Logging {
def process(
command: proto.Command,
- responseObserver: StreamObserver[ExecutePlanResponse],
- executeHolder: ExecuteHolder): Unit = {
+ responseObserver: StreamObserver[ExecutePlanResponse]): Unit = {
Review Comment:
FWIW, between Spark 3.4 and Spark 3.5, these interfaces also changed.
For example https://github.com/apache/spark/pull/41618 made it take a SessionHolder instead of SparkSession, so that various parameters don't have to be passed to it lossely. This PR adding ExecuteHolder to it is in a very similar spirit.
--
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-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #43311:
URL: https://github.com/apache/spark/pull/43311#discussion_r1355364109
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -86,14 +86,29 @@ final case class InvalidCommandInput(
private val cause: Throwable = null)
extends Exception(message, cause)
-class SparkConnectPlanner(val sessionHolder: SessionHolder) extends Logging {
+class SparkConnectPlanner(
+ val sessionHolder: SessionHolder,
+ val executeHolderOpt: Option[ExecuteHolder] = None)
Review Comment:
Ok, makes sense.
--
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-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell closed pull request #43311: [SPARK-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner
URL: https://github.com/apache/spark/pull/43311
--
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-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #43311:
URL: https://github.com/apache/spark/pull/43311#discussion_r1355315695
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -86,14 +86,29 @@ final case class InvalidCommandInput(
private val cause: Throwable = null)
extends Exception(message, cause)
-class SparkConnectPlanner(val sessionHolder: SessionHolder) extends Logging {
+class SparkConnectPlanner(
+ val sessionHolder: SessionHolder,
+ val executeHolderOpt: Option[ExecuteHolder] = None)
Review Comment:
Why not just pass in the ExecuteHolder, and create a getter for SessionHolder? IMO it is a bit weird to create this structure for just a single unit test.
--
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-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #43311:
URL: https://github.com/apache/spark/pull/43311#discussion_r1355319835
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -2461,48 +2475,39 @@ class SparkConnectPlanner(val sessionHolder: SessionHolder) extends Logging {
def process(
command: proto.Command,
- responseObserver: StreamObserver[ExecutePlanResponse],
- executeHolder: ExecuteHolder): Unit = {
+ responseObserver: StreamObserver[ExecutePlanResponse]): Unit = {
Review Comment:
No, I don't think we should. In the end that will hamper our ability to evolve the interface.
--
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-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #43311:
URL: https://github.com/apache/spark/pull/43311#issuecomment-1758090274
Merging.
--
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