viirya opened a new issue, #7102: URL: https://github.com/apache/datafusion-comet/issues/7102
### Describe the bug When a native sort cannot reserve memory, the task fails with DataFusion's message from `ExternalSorter::err_with_oom_context` (`datafusion-physical-plan` 55.2, `src/sorts/sort.rs`): ``` Not enough memory to continue external sort. Consider increasing the memory limit config: 'datafusion.runtime.memory_limit', or decreasing the config: 'datafusion.execution.sort_spill_reservation_bytes'. caused by Resources exhausted: Failed to acquire ... bytes where this consumer already holds ... bytes and the fair limit is ... bytes, ... registered (... bytes overcommitted) ``` Comet plans sorts with DataFusion's `SortExec` (`native/core/src/execution/planner.rs`, `planner/aggregate.rs`), so users can see this. Neither key is something they can act on: - `datafusion.runtime.memory_limit` is not used. `prepare_datafusion_session_context` (`native/core/src/execution/jni_api.rs`) passes Comet's own memory pool to the `RuntimeEnvBuilder`. The pool is built from `spark.memory.offHeap.size` and `spark.comet.exec.memoryPool` (`CometExecIterator.getMemoryConfig`, `memory_pools/config.rs`), and its reservations go through Spark's `TaskMemoryManager`, so a task gets a share of the off-heap pool, not all of it. The error can only come up in off-heap mode: in on-heap mode the pool is `UnboundedMemoryPool` and never refuses a reservation. - `datafusion.execution.sort_spill_reservation_bytes` is never set in production code, so it stays at DataFusion's 10 MiB default. Users can only reach it through `spark.comet.datafusion.execution.sort_spill_reservation_bytes`, and the JVM forwards `spark.comet.datafusion.*` keys only when the testing-only `spark.comet.exec.respectDataFusionConfigs` is on (`CometExecIterator.serializeCometSQLConfs`). This is the same kind of problem as #7091, where the spill-limit error names `datafusion.runtime.max_temp_directory_size`. A sweep of DataFusion 55.2 for error text that tells users to change `datafusion.*` keys turned up only these two messages. ### Proposed fix Keep DataFusion's sentence and add a Comet hint after it, the way #7098 does for the spill limit. For example: > Comet does not use `datafusion.runtime.memory_limit` or `datafusion.execution.sort_spill_reservation_bytes`: native operators reserve memory from Spark's off-heap pool, sized by `spark.memory.offHeap.size`, and each running task gets a share of it. Raise `spark.memory.offHeap.size` or run fewer tasks per executor; see the Comet memory tuning guide. The wording is open to discussion. In particular, it might also be worth mentioning `spark.comet.exec.memoryPool=greedy_unified` when the plan has only one spillable operator. That advice is in [Memory Tuning](https://datafusion.apache.org/comet/user-guide/latest/tuning/memory.html), but it may be too situational for an error message. ### Where to hook it `natural_exception` in `native/jni-bridge/src/errors.rs`, next to `with_spill_limit_hint` from #7098. The sort error arrives as `CometError::DataFusion { source: DataFusionError::Context(_, ResourcesExhausted(_)) }`, sometimes inside `Shared`. `jvm_exception` sends that through `generic_exception` to `natural_exception`, and the `External` fallback also ends there. The text survives: `From<DataFusionError> for CometError` stores `DataFusionError::message()`, which formats a `Context` as `"{desc}\ncaused by\n{inner}"`, and `to_exception` uses that string for the `CometNativeException`. Unlike the spill-limit error, this message quotes the keys with single quotes (`'datafusion.runtime.memory_limit'`), so the match should be on that exact quoted text. The match is on text because the context string is the only thing that identifies the sort. If #7098 is merged first, its single-key check could become a small list of (DataFusion text, Comet hint) pairs, so the two hints share one place. ### Testing Get the error from a real `SortExec`, not a copied string, so a rewording in a DataFusion upgrade fails the test instead of silently dropping the hint. Run a `SortExec` over a small in-memory batch in a `TaskContext` whose `RuntimeEnv` has a memory pool smaller than the batch (for example `GreedyMemoryPool::new(1)`). `insert_batch` then fails on its first reservation, either the merge headroom (`reserve_memory_for_merge`, when temp files are enabled) or the batch itself while no batches are buffered. Both paths go through `err_with_oom_context`, and neither has to spill. Then pass the error through `CometError::from` / `classify` and check: - the class is `CometNativeException`, and the message is DataFusion's text with the hint appended, - a `Shared`-wrapped copy gets the hint too, - a plain `ResourcesExhausted` that has no sort context is left unchanged. `datafusion-comet-jni-bridge` has no async runtime among its dev-dependencies, so the test would need `tokio` (it's already a workspace dependency) or would have to live in `native/core`. ### Related - #7091 / #7098: the same problem for `datafusion.runtime.max_temp_directory_size`. -- 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]
