andygrove opened a new issue, #7312:
URL: https://github.com/apache/datafusion-comet/issues/7312
## Describe the bug
`CapturedSQLConf.withConf`, added in #7302, decides that Spark already
passed the task the session's configs by checking whether any captured
`spark.comet.*` key is a task local property. Spark only propagates the keys
that are currently set in the session. If every key in the capture has since
been unset, a task inside a SQL execution finds none of them, concludes Spark
passed nothing, and applies the stale capture over the session's real configs.
The capture lives in cached objects (`shuffleDependency` is a lazy val on
the cached `executedPlan`, and the RDDs capture in their constructors), so it
can outlive the setting. It needs a map stage to run again after the session
changed, for example after a fetch failure, so this is narrow. It also needs
every captured key to be unset. The session `SQLConf` carries the
`spark.comet.*` keys from the SparkConf, so a deployment that sets any of them
there is not affected. When the capture holds only keys set at runtime with
`spark.conf.set`, it is.
## Steps to reproduce
With a capture that holds only `spark.comet.x=1`, unset `x` and set `y=2` on
the session, then read `SQLConf.get` in a task inside
`SQLExecution.withNewExecutionId`:
- Expected: `x` is unset and `y` is `2`.
- Actual: `x` is `1` and `y` is unset.
`CapturedSQLConf.capture()` reads the active `SQLConf`, so a test can build
that capture deterministically with `SQLConf.withExistingConf(new SQLConf())`.
## Expected behavior
Inside a SQL execution the task sees the session's configs, as it does for
every other Spark config.
## Additional context
Every Spark path that sets `SQLExecution.EXECUTION_ID_KEY` also calls
`withSQLConfPropagated` (checked on 3.4.3, 3.5.8, 4.0.4, 4.1.3 and 4.2.0), so
skipping `withConf` when the id is set is exact. The existing key check stays
for `withSQLConfPropagated` on its own, which sets the configs but no id.
```scala
val context = TaskContext.get()
if (configs.isEmpty || context == null ||
!SQLConf.get.isInstanceOf[ReadOnlySQLConf] ||
+ context.getLocalProperty(SQLExecution.EXECUTION_ID_KEY) != null ||
configs.keys.exists(context.getLocalProperty(_) != null)) {
```
I ran this against the head of #7302 (`bcf48626d5`) with a new
`CapturedSQLConfSuite` of five cases, one per skip condition. The two
stale-capture cases fail without the line above and all five pass with it. The
suite needs a capture that holds only the key under test, because the test
session carries other `spark.comet.*` keys and the stale case then passes
vacuously.
Raised in the review of #7302 as an optional nit.
--
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]
