New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-43474] [SS] [CONNECT] Add SessionHolder to SparkConnectPlanner #41618
Conversation
@@ -79,7 +79,10 @@ final case class InvalidCommandInput( | |||
private val cause: Throwable = null) | |||
extends Exception(message, cause) | |||
|
|||
class SparkConnectPlanner(val session: SparkSession) extends Logging { | |||
class SparkConnectPlanner(val sessionHolder: SessionHolder) extends Logging { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change. The rest of the changes are consequence of API change.
@grundprinzip, @HyukjinKwon PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it needs some reviews @vicennial and/or @hvanhovell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this refactoring!
@@ -51,11 +52,12 @@ trait SparkConnectPlanTest extends SharedSparkSession { | |||
} | |||
|
|||
def transform(rel: proto.Relation): logical.LogicalPlan = { | |||
new SparkConnectPlanner(spark).transformRelation(rel) | |||
new SparkConnectPlanner(SessionHolder.forTesting(spark)).transformRelation(rel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
Merging. |
### What changes were proposed in this pull request? This adds SessionHolder rather than just SparkSession to `SparkConnectPlanner`. This is to allow access to session specific state at connect server level. Note that this is Spark-Connect specific session state, and is not stored with SparkSession. E.g. * Mapping from _dataframe reference id_ to actual dataframe in apache#41580 * File and archives stored with session in apache#41495 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Existing unit tests. Closes apache#41618 from rangadi/session-holder. Authored-by: Raghu Angadi <raghu.angadi@databricks.com> Signed-off-by: Herman van Hovell <herman@databricks.com>
Thanks for the refactoring! |
What changes were proposed in this pull request?
This adds SessionHolder rather than just SparkSession to
SparkConnectPlanner
. This is to allow access to session specific state at connect server level. Note that this is Spark-Connect specific session state, and is not stored with SparkSession.E.g.
Does this PR introduce any user-facing change?
No.
How was this patch tested?