andygrove commented on code in PR #5634:
URL: https://github.com/apache/datafusion-comet/pull/5634#discussion_r4096081979
##########
spark/src/main/scala/org/apache/spark/Plugins.scala:
##########
@@ -104,7 +104,9 @@ object CometDriverPlugin extends Logging {
private[apache] def maybeSetCacheSerializer(
conf: SparkConf,
extraConfs: ju.HashMap[String, String]): Unit = {
- if (conf.getBoolean(CometConf.COMET_EXEC_IN_MEMORY_CACHE_ENABLED.key,
false)) {
+ if (conf.getBoolean(
+ CometConf.COMET_EXEC_IN_MEMORY_CACHE_ENABLED.key,
+ CometConf.COMET_EXEC_IN_MEMORY_CACHE_ENABLED.defaultValue.get)) {
Review Comment:
Agreed. With the default on, an application that can never plan the native
scan shouldn't get Comet's format. I'll gate the install on
`spark.comet.enabled` and `spark.comet.exec.enabled` through `getBooleanConf`,
and add the three plugin cases, before this comes out of draft. It won't avoid
the slow path on its own, though: with both on, AQE still puts Spark operators
above the cache scan, which is #6202.
##########
spark/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -278,7 +278,7 @@ object CometConf extends ShimCometConf {
"SparkContext, otherwise caching fails as soon as a block is
serialized, including " +
"the disk half of the default MEMORY_AND_DISK storage level.")
.booleanConf
- .createWithDefault(false)
+ .createWithDefault(true)
Review Comment:
The audit turned up part of the answer. Under AQE the first case is the
normal outcome, not an edge case. Once the table-cache stage materializes, the
re-plan leaves the operators above it on Spark, so a plain aggregate or join
over a cached table reads Comet's format through a `CometColumnarToRow`
(#6202). `CometInMemoryCacheBenchmark` runs with AQE off, so the published
numbers don't show it. I'll fix #6202 first and then benchmark with AQE on
against Spark's own format, so the number measures the path users will actually
get.
Yes to option 1 from #5485 landing with the default flip. It will need to
cover the #6202 path too, since nothing records a fallback reason there today.
##########
docs/source/user-guide/latest/in-memory-cache.md:
##########
@@ -24,13 +24,13 @@ format that Comet operators read directly. Without it, a
cached table is stored
format and every scan of it has to convert each batch before Comet can
continue, which shows up in
the plan as a `CometSparkColumnarToColumnar` above the cache scan.
-This feature is **experimental and disabled by default**. Turn it on at
startup, alongside the rest
-of Comet's configuration:
+This feature is **experimental and enabled by default**. To turn it off, set
the config at startup,
+alongside the rest of Comet's configuration:
Review Comment:
Agreed on the upgrade guide entry, covering both the format change and the
Kryo requirement. Moving the flip past 1.1.0 changes one premise, though. 1.1.0
ships this key with a default of `false`, so turning it on in the next release
is a change to an existing key's default, which is the first case the policy
lists. Let's settle the legacy-key question when this comes out of draft.
--
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]
