RamonZhou commented on code in PR #58264: URL: https://github.com/apache/spark/pull/58264#discussion_r3867141799
########## sql/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala: ########## @@ -87,6 +87,15 @@ import org.apache.spark.unsafe.types.UTF8String import org.apache.spark.util.ArrayImplicits._ import org.apache.spark.util.Utils +/** + * Translates a Spark Connect request into Catalyst. + * + * An instance is request-scoped: construct one per request and discard it. Some state is derived + * once and reused for the whole request -- notably the Python worker environment, which must be a + * single snapshot so that a plan cannot be built with one environment and cached under another. + * Reusing an instance across requests would pin that state to whatever the first request Review Comment: Planner are constructed per-request in tree, so this issue will not happen; it only happens when the user constructs a Planner by themself, calls `transformRelation` multiple times, and mutates the env vars in between. The first suggestion will break correctness (capture the environment per top-level transform) may cause a plan being built under a set of env vars but being cached under another set of env vars. This is a bigger issue in my opinion. The second suggestion may work but requires a lot of refactoring and potentially changing APIs. I prefer document this as an accepted risk (as mentioned in the comments here), env vars will be captured only once per instance, and will be used for the whole request/plan. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
