viirya commented on code in PR #7104:
URL: https://github.com/apache/datafusion-comet/pull/7104#discussion_r4316409714
##########
native/jni-bridge/src/errors.rs:
##########
@@ -808,18 +808,54 @@ fn generic_exception(error: &CometError, backtrace:
Option<&str>) -> JvmExceptio
/// message as a stack trace when available.
fn natural_exception(error: &CometError, backtrace: Option<&str>) ->
JvmException<'static> {
let exception = error.to_exception();
+ let msg = with_comet_config_hint(exception.msg);
let message = match backtrace {
- Some(backtrace_string) => {
- to_stacktrace_string(exception.msg,
backtrace_string.to_string()).unwrap()
- }
- None => exception.msg,
+ Some(backtrace_string) => to_stacktrace_string(msg,
backtrace_string.to_string()).unwrap(),
+ None => msg,
};
JvmException::New {
class: exception.class,
message,
}
}
+/// The config key DataFusion's spill-limit error tells users to raise. Comet
sets the limit
+/// from `spark.comet.maxTempDirectorySize` instead, and DataFusion raises the
error as a plain
+/// `std::io::Error`, so the key in the message text is the only thing to
match on.
+const DATAFUSION_SPILL_LIMIT_KEY: &str =
"`datafusion.runtime.max_temp_directory_size`";
+
+const SPILL_LIMIT_HINT: &str = "Comet does not use `datafusion.runtime.*`
settings: \
+ the spill limit is `spark.comet.maxTempDirectorySize`. It applies to each
Comet native plan \
+ separately, so an executor can use several times that much disk.";
+
+/// The first config key DataFusion's external-sort OOM error tells users to
change. Comet never
+/// uses it, and the sort is identified only by the `Context` string around
`ResourcesExhausted`,
+/// so this quoted text is what to match on.
+const DATAFUSION_SORT_MEMORY_KEY: &str = "'datafusion.runtime.memory_limit'";
+
+const SORT_MEMORY_HINT: &str = "Comet does not use
`datafusion.runtime.memory_limit` or \
Review Comment:
Thanks for picking this up so quickly. Getting the error from a real
`SortExec` is a nice way to keep the match honest across DataFusion upgrades.
One correction to the hint, and it came from the wording I proposed in
#7102. It says Comet does not use
`datafusion.execution.sort_spill_reservation_bytes`, but the sort does use it.
Comet builds its `DiskManager` in `Directories` mode, so `tmp_files_enabled()`
is true and `ExternalSorter::reserve_memory_for_merge` reserves
`sort_spill_reservation_bytes` on every `insert_batch`, through its own
`ExternalSorterMerge[N]` consumer. Comet never sets the key, so that is
DataFusion's 10 MiB default. Under `fair_unified` that consumer also counts
toward the number of consumers the pool divides by, so when a task's fair share
falls below 10 MiB this merge reservation is the one that fails. What users
can't do is change it, since `spark.comet.datafusion.*` only reaches native
code with the testing-only `spark.comet.exec.respectDataFusionConfigs`.
Could the hint say something closer to this?
"Comet does not use `datafusion.runtime.memory_limit`, and it keeps
`datafusion.execution.sort_spill_reservation_bytes` at DataFusion's default of
10 MiB. 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 PR description's "Comet uses neither" could use the same fix.
--
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]
