You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "tomvanbussel (via GitHub)" <gi...@apache.org> on 2024/05/03 09:20:34 UTC

[PR] [SPARK-48112][CONNECT] Expose session in SparkConnectPlanner to plugins [spark]

tomvanbussel opened a new pull request, #46363:
URL: https://github.com/apache/spark/pull/46363

   ### What changes were proposed in this pull request?
   
   This PR makes `session` in `SparkConnectPlanner` public so that it can be accessed by Spark Connect plugins. This method was public before but was made private. To prevent this from happening I have added `@DeveloperApi` as an annotation.
   
   ### Why are the changes needed?
   
   To allow the development of Spark Connect plugins.
   
   
   ### Does this PR introduce _any_ user-facing change?
   
   No
   
   
   ### How was this patch tested?
   
   N/A
   
   
   ### 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-48112][CONNECT] Expose session in SparkConnectPlanner to plugins [spark]

Posted by "tomvanbussel (via GitHub)" <gi...@apache.org>.
tomvanbussel commented on code in PR #46363:
URL: https://github.com/apache/spark/pull/46363#discussion_r1591488839


##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -101,7 +101,8 @@ class SparkConnectPlanner(
     throw new IllegalArgumentException("executeHolder does not belong to sessionHolder")
   }
 
-  private[connect] def session: SparkSession = sessionHolder.session
+  @DeveloperApi

Review Comment:
   Done!



-- 
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-48112][CONNECT] Expose session in SparkConnectPlanner to plugins [spark]

Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #46363: [SPARK-48112][CONNECT] Expose session in SparkConnectPlanner to plugins
URL: https://github.com/apache/spark/pull/46363


-- 
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-48112][CONNECT] Expose session in SparkConnectPlanner to plugins [spark]

Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46363:
URL: https://github.com/apache/spark/pull/46363#discussion_r1589021943


##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -101,7 +101,8 @@ class SparkConnectPlanner(
     throw new IllegalArgumentException("executeHolder does not belong to sessionHolder")
   }
 
-  private[connect] def session: SparkSession = sessionHolder.session
+  @DeveloperApi
+  def session: SparkSession = sessionHolder.session

Review Comment:
   I am fine with this for now but I really think we should probably make `SparkConnectPlanner` an API first



-- 
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-48112][CONNECT] Expose session in SparkConnectPlanner to plugins [spark]

Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #46363:
URL: https://github.com/apache/spark/pull/46363#issuecomment-2097136188

   Merged to master.


-- 
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-48112][CONNECT] Expose session in SparkConnectPlanner to plugins [spark]

Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #46363:
URL: https://github.com/apache/spark/pull/46363#issuecomment-2097265334

   Thank you. +1, LGTM.


-- 
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-48112][CONNECT] Expose session in SparkConnectPlanner to plugins [spark]

Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #46363:
URL: https://github.com/apache/spark/pull/46363#discussion_r1589376607


##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -101,7 +101,8 @@ class SparkConnectPlanner(
     throw new IllegalArgumentException("executeHolder does not belong to sessionHolder")
   }
 
-  private[connect] def session: SparkSession = sessionHolder.session
+  @DeveloperApi

Review Comment:
   Could you add a `Since` tag, @tomvanbussel ?



-- 
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