ump45nose opened a new pull request, #7104: URL: https://github.com/apache/datafusion-comet/pull/7104
## Which issue does this PR close? Closes #7102. This builds on #7098 (the spill-limit hint), so the first commit here is #7098's. Only the last commit is new; I'll rebase onto `main` once #7098 merges. ## Rationale for this change When a native sort can't reserve memory, the task fails with DataFusion's external-sort message, which tells users to raise `'datafusion.runtime.memory_limit'` or lower `'datafusion.execution.sort_spill_reservation_bytes'`. Comet uses neither: native operators reserve from Spark's off-heap pool, so the error points users at settings that do nothing. ## What changes are included in this PR? As @viirya suggested on the issue, #7098's single-key check in `native/jni-bridge/src/errors.rs` becomes a small list of (DataFusion text, Comet hint) pairs, and `natural_exception` appends the hint for the first match. The new entry matches the single-quoted `'datafusion.runtime.memory_limit'` text from DataFusion's `ExternalSorter::err_with_oom_context`. The DataFusion message is kept, and the hint uses the wording proposed in the issue: Comet doesn't use those two keys, memory comes from the off-heap pool sized by `spark.memory.offHeap.size` and shared between running tasks, so raise that or run fewer tasks per executor, and see the memory tuning guide. I left out the `greedy_unified` advice since the issue flagged it as probably too situational. ## How are these changes tested? `sort_oom_error_names_the_comet_config` gets the error from a real `SortExec` over a small in-memory batch, with a `GreedyMemoryPool` of 1 byte, rather than a copied string, so a rewording in a DataFusion upgrade fails the test instead of silently dropping the hint. It checks the full JVM message and the `CometNativeException` class for the plain error and a `Shared`-wrapped copy, and checks that a `ResourcesExhausted` error without the sort context is unchanged. The test fails without the change. The stream is polled with `futures::executor::block_on`, so `futures` (already a workspace dependency) is added to the crate's dev-dependencies instead of a tokio runtime. I ran the `datafusion-comet-jni-bridge` lib tests (the four tests that start a JVM need the Maven-built `comet-common` classes, which I didn't build; they fail the same way without this change) and `cargo clippy --all-targets -- -D warnings` on that crate. AI disclosure: the change, the test and this description were written with an AI coding assistant. The hint wording, and whether matching on the quoted key is narrow enough, are the parts most worth a look. -- 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]
