RamonZhou commented on code in PR #58264: URL: https://github.com/apache/spark/pull/58264#discussion_r3867175338
########## 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: In fact, none of the configs are included in the plan cache key nowadays. So if a `spark.conf` field is changed and the same request was run again, it will use the stale config values from a cache hit. So for env vars, we already treat them as a config value. Maybe we can adopt the behavior and exclude them in the plan cache key? The env vars probably won't change frequently, except maybe for rotating secrets. -- 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]
