andygrove commented on PR #7302: URL: https://github.com/apache/datafusion-comet/pull/7302#issuecomment-6499263803
I checked the stale-capture nit and it is real, but narrow enough that I don't want to hold this PR for it. With a capture holding only `spark.comet.x`, if the session unsets `x` and sets `y` afterwards, a task inside a SQL execution finds no captured key among its local properties and applies the stale capture, so it reads `x=1` and misses `y=2`. It needs every captured key to be unset, and the session `SQLConf` also carries the `spark.comet.*` keys from the SparkConf, so most deployments have a key that stays set. It also needs a map stage to rerun after the change. Skipping `withConf` when `SQLExecution.EXECUTION_ID_KEY` is set makes it exact. I read `SQLExecution` on 3.4.3, 3.5.8, 4.0.4, 4.1.3 and 4.2.0, and every path that sets the id also propagates the session's configs. I wrote the unit test for the skip conditions, and the two stale cases fail on this head and pass with the one-line change. I filed #7312 with the details and the diff, so I'll queue this one as it is and take the fix there. -- 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]
