andygrove commented on code in PR #6195:
URL: https://github.com/apache/datafusion-comet/pull/6195#discussion_r4095487423
##########
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala:
##########
@@ -292,6 +299,12 @@ object CometSparkSessionExtensions extends Logging {
sparkConf.getBoolean("spark.memory.offHeap.enabled", false)
}
+ // Spark copies the SparkConf into each session's SQLConf and, by default,
refuses to set core
+ // configs such as this one at session level, so this is the application's
memory mode.
+ private def isOffHeapEnabled(conf: SQLConf): Boolean = {
+ conf.getConfString("spark.memory.offHeap.enabled", "false").toBoolean
+ }
Review Comment:
Good catch, thanks. I reproduced it: with the suite's session already
running, `SparkSession.builder().config("spark.memory.offHeap.enabled",
"true").getOrCreate()` put `true` in the session's `SQLConf` while `SparkEnv`
still said `false`, and the query planned `CometNativeScan` and `CometFilter`.
A new session built on an already-running `SparkContext` has the same problem,
because `BaseSessionStateBuilder` merges the builder's options into its
`SQLConf` with `mergeNonStaticSQLConfigs`.
Fixed in 2f207b0f2, with two differences from the suggestion:
- The `SQLConf` overload is gone rather than kept with a parameter it
ignores. `isCometLoaded` passes `SparkEnv.get.conf` to the existing
`isOffHeapEnabled`, guarding against a null `SparkEnv`.
`spark.comet.exec.onHeap.enabled` still comes from the `SQLConf`, so
`ENABLE_COMET_ONHEAP` still supplies its default.
- The unit test I had added to `CometSparkSessionExtensionsSuite` set
off-heap memory on a bare `SQLConf`, which the check now ignores, and that
suite's `SparkContext` runs off-heap, so committing the suggestion as is would
have failed it. I removed that test and its helper, which puts the suite back
to what is on main. `CometPluginsExtensionOnlySuite` covers the gate instead
and now has the test you proposed, going through the builder path rather than
`withSQLConf`.
--
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]
