andygrove opened a new pull request, #6287: URL: https://github.com/apache/datafusion-comet/pull/6287
## Which issue does this PR close? Closes #6259. ## Rationale for this change `spark.comet.shuffle.jvm.batchSize=0` passed the only check the entry had, that it is no larger than `spark.comet.batchSize`. The JVM shuffle writer then hands 0 to `process_sorted_row_partition`, whose loop never advances, and since the loop runs inside a JNI call the task hangs and can't be killed. `spark.comet.exec.memoryPool` had no check at all, so a misspelled value, or one that differs only in case, reached native code, and in off-heap mode every task failed with `Unsupported memory pool type` when it created a native plan. ## What changes are included in this PR? - `spark.comet.shuffle.jvm.batchSize` must be positive. The new check runs ahead of the existing one. - `spark.comet.exec.memoryPool` is lowercased and must be `fair_unified` or `greedy_unified`, the same way `spark.comet.shuffle.mode` is handled. `getMemoryConfig` reads it through the entry, so native code gets the lowercased value. Comet checks a config when it is read, and both of these are read in the executor task, so a bad value still fails the task. It now fails straight away, with an error that names the config and the values it accepts. Rejecting a value when it is set would mean checking Comet's configs on the driver, which is a bigger change than this. The existing check on `spark.comet.shuffle.jvm.batchSize` has a problem of its own, filed as #6286. It reads `spark.comet.batchSize` while `CometConf` is being initialized, so a batch size below 8192 stops `CometConf` from initializing on executors. This PR leaves that check as it is. ## How are these changes tested? - Two new tests in `CometConfSuite`. The first checks that 0 and -1 are rejected as the batch size and 1 is accepted. The second checks that the pool type is lowercased and that a misspelling is rejected with the valid values in the message. Both fail on main. - End to end, with a suite that isn't part of this PR. On main, a batch size of 0 hung in `Native.writeSortedFileNative` on both the bypass-merge path (4 partitions) and the sort path (300 partitions), and `GREEDY_UNIFIED`, `Fair_Unified` and `fair` each failed every task with `Unsupported memory pool type`. With this change the batch size of 0 fails the task within a second, the two case variants run natively with answers that match Spark, and `fair` fails with `should be one of fair_unified, greedy_unified, but was fair`. A test of the hang itself would hang CI rather than fail, so that one isn't committed. -- 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]
