kazuyukitanimura commented on code in PR #6698:
URL: https://github.com/apache/datafusion-comet/pull/6698#discussion_r4190145284
##########
docs/source/user-guide/latest/tuning.md:
##########
@@ -36,6 +36,20 @@ guide is split into the following pages:
- [Reducing Row/Columnar Conversion Overhead](tuning/transitions.md): stages
in which many
operators fall back to Spark.
+## Nested and Wide Data
+
+Rows with many columns, long strings, or nested columns such as arrays of
structs make every batch
+larger, so queries over such data spill more. For these workloads:
+
+- Compress spill files, which Comet writes uncompressed by default. See
Review Comment:
Native shuffle already compresses its spills with the shuffle codec, and
memory.md says so. Could this bullet say something like "Compress spill files
from native sorts, aggregations, and joins, which DataFusion writes
uncompressed by default"? Otherwise readers with wide or nested data may expect
it to affect shuffle spill too.
##########
spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala:
##########
@@ -110,6 +110,21 @@ class CometExecSuite extends CometTestBase {
}
}
+ test("SQLConf serde passes allowed DataFusion configs without
respectDataFusionConfigs") {
+ def entries =
ConfigMap.parseFrom(CometExecIterator.serializeCometSQLConfs()).getEntriesMap
+ val spillCompression = "spark.comet.datafusion.execution.spill_compression"
+ val spillReservation =
"spark.comet.datafusion.execution.sort_spill_reservation_bytes"
+
+ withSQLConf(spillCompression -> "zstd", spillReservation -> "65536") {
+ assert(entries.get(spillCompression) == "zstd")
+ assert(!entries.containsKey(spillReservation))
+
+ withSQLConf(CometConf.COMET_ALLOWED_DATAFUSION_CONFIGS.key ->
spillReservation) {
Review Comment:
Since the doc says setting the list replaces the default, could the inner
block also assert !entries.containsKey(spillCompression)? As written, it would
still pass if the list were appended to the default.
##########
spark/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -1259,10 +1259,23 @@ object CometConf extends ShimCometConf {
.doc(
"Development and testing configuration option to allow DataFusion
configs set in " +
"Spark configuration settings starting with
`spark.comet.datafusion.` to be passed " +
- "into native execution.")
+ "into native execution. Configs listed in " +
+ "`spark.comet.exec.allowedDataFusionConfigs` are passed even when
this is false.")
.booleanConf
.createWithDefault(false)
+ val COMET_ALLOWED_DATAFUSION_CONFIGS: ConfigEntry[Seq[String]] =
Review Comment:
Thanks for chasing this down, the q21 numbers make a great case for exposing
spill compression. I'm wondering about the shape though. Because this list
lives in the tuning category, the versioning policy covers it, and per
config_conventions.md so does the documented
spark.comet.datafusion.execution.spill_compression key. Its accepted values
and default come from DataFusion's SpillCompression, so a DataFusion
upgrade could change a key we've promised to keep stable. Users can also list
any spark.comet.datafusion.* key here, as the new test does with
sort_spill_reservation_bytes. That makes it a supported per-key way around
respectDataFusionConfigs. Would a dedicated Comet key work
better, for example spark.comet.exec.spill.compression with
checkValues(Set("uncompressed", "lz4_frame", "zstd"))? It could be translated
in prepare_datafusion_session_context the same way
COMET_PARQUET_ROW_FILTER_PUSHDOWN_ENABLED is, and added to the resolved list in
serializeCometSQLConfs. That would also catch a typo like lz4 on the driver.
Today it panics in SessionConfig::set_str on every task.
--
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]
