comphead commented on code in PR #7104:
URL: https://github.com/apache/datafusion-comet/pull/7104#discussion_r4329263992
##########
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`, and in \
+ production 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.";
Review Comment:
The hint ends with "see the Comet memory tuning guide", and in a Spark task
failure message there is nothing to follow. `CometConf` already embeds
`https://datafusion.apache.org/comet/user-guide/latest/tuning.html` in its
docs, so there is precedent for a URL. Could the hint include a link? I could
not check that a more specific page resolves from here, so the `tuning.html`
one already used in the repo is the safer choice.
##########
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`, and in \
+ production 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.";
+
+/// DataFusion error text that names settings unavailable for production
tuning in Comet,
+/// paired with the hint that points users at the Comet settings to change
instead.
+const COMET_CONFIG_HINTS: &[(&str, &str)] = &[
+ (DATAFUSION_SPILL_LIMIT_KEY, SPILL_LIMIT_HINT),
+ (DATAFUSION_SORT_MEMORY_KEY, SORT_MEMORY_HINT),
+];
+
+fn with_comet_config_hint(message: String) -> String {
+ match COMET_CONFIG_HINTS
+ .iter()
+ .find(|(datafusion_key, _)| message.contains(datafusion_key))
+ {
+ Some((_, hint)) => format!("{message} {hint}"),
Review Comment:
This joins the message and the hint with one space, but the DataFusion text
it follows ends mid-sentence. The sort error is a `Context` whose message is
`{desc}\ncaused by\n{inner}`, and the inner pool refusal ends in `... Reserved:
N` or `... Reserved: N bytes` (`unified_pool.rs` and `fair_pool.rs`). By my
reading the user sees `... Reserved: 0 Comet does not use ...`. Could the join
be a newline or `. ` so the hint starts its own sentence? The same line serves
the spill-limit hint, and the tests build their expectation with the same
`format!("{error} {HINT}")`, so they would need the same separator. I read this
from the sources and did not run it.
--
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]
