andygrove opened a new issue, #6184: URL: https://github.com/apache/datafusion-comet/issues/6184
### Describe the bug `spark.comet.maxTempDirectorySize` is declared as a byte size ([CometConf.scala#L1099-L1109](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/main/scala/org/apache/comet/CometConf.scala#L1099-L1109)), but nothing on the JVM resolves it. The raw session string is sent to native code, which parses it with `str.parse::<u64>()` and falls back to the 100 GiB default when that fails ([jni_api.rs#L590-L591](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/native/core/src/execution/jni_api.rs#L590-L591), [spark_config.rs#L42-L46](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/native/core/src/execution/spark_config.rs#L42-L46)). Any value with a unit is ignored without a warning: `10g`, `500m`, and even `107374182400b`, the form the generated config table uses for the default. The same path makes the boolean flags that native code reads case-sensitive. `spark.comet.debug.memory`, `spark.comet.tracing.enabled`, `spark.comet.debug.enabled` and `spark.comet.explain.native.enabled` go through `get_bool` ([spark_config.rs#L36-L40](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/native/core/src/execution/spark_config.rs#L36-L40)), which accepts only lowercase `true`. The JVM accepts `TRUE`, so the two sides can disagree about whether a flag such as tracing is on. The documented scope of the limit is also wrong. The config doc ([CometConf.scala#L1105](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/main/scala/org/apache/comet/CometConf.scala#L1105)) and the tuning guide ([tuning.md#L287](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/docs/source/user-guide/latest/tuning.md#L287)) say it applies per Spark task. Each `createPlan` builds its own `DiskManager` with the limit ([jni_api.rs#L835](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/native/core/src/execution/jni_api.rs#L835)), so it applies per native plan. A task that runs a pre-shuffle plan and a shuffle writer plan gets the limit twice. ### Steps to reproduce `get_u64` returns the 100 GiB default for a map containing `spark.comet.maxTempDirectorySize = "1m"`, because `"1m".parse::<u64>()` fails. So with `spark.comet.maxTempDirectorySize=1m` a native sort can spill far past 1 MiB, while `1048576` stops it at 1 MiB. ### Expected behavior Values with a unit are honored, or rejected with an error, and the boolean flags accept the same spellings on both sides. ### Additional context Suggested fix: resolve these configs on the JVM and put the typed values in the map that `serializeCometSQLConfs` sends, as it already does for `spark.comet.parquet.rowFilterPushdown.enabled` ([CometExecIterator.scala#L600-L605](https://github.com/apache/datafusion-comet/blob/67803a7a422c44de07af1e5d25c1dbeae8df68d4/spark/src/main/scala/org/apache/comet/CometExecIterator.scala#L600-L605)). That also sends the defaults explicitly instead of repeating them in Rust. Then correct "per Spark task" in the config doc and the tuning guide. -- 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]
