[SPARK-45204][CONNECT] Add optional ExecuteHolder to SparkConnectPlanner #43311
Conversation
| def process( | ||
| command: proto.Command, | ||
| responseObserver: StreamObserver[ExecutePlanResponse], | ||
| executeHolder: ExecuteHolder): Unit = { | ||
| responseObserver: StreamObserver[ExecutePlanResponse]): Unit = { |
There was a problem hiding this 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?
There was a problem hiding this comment.
FWIW, between Spark 3.4 and Spark 3.5, these interfaces also changed.
For example #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.
There was a problem hiding this comment.
No, I don't think we should. In the end that will hamper our ability to evolve the interface.
| class SparkConnectPlanner(val sessionHolder: SessionHolder) extends Logging { | ||
| class SparkConnectPlanner( | ||
| val sessionHolder: SessionHolder, | ||
| val executeHolderOpt: Option[ExecuteHolder] = None) |
There was a problem hiding this 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.
There was a problem hiding this comment.
it's used in AnalyzePlan that doesn't have an ExecuteHolder
hvanhovell
left a comment
hvanhovell
left a comment
There was a problem hiding this comment.
LGTM - we can address the constructor comment in a follow-up.
|
Merging. |
4 participants
What changes were proposed in this pull request?
This PR adds an optional
ExecuteHoldertoSparkConnectPlanner.This allows plugins to access the
ExecuteHolderto for example create aQueryPlanningTrackerwithout the need to create a newCommandPlugininterface like proposed in #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.