viirya commented on code in PR #7104:
URL: https://github.com/apache/datafusion-comet/pull/7104#discussion_r4321722923
##########
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 the update. The new wording reads well and matches what the sort
actually does.
The `assert_eq!` on
`ConfigOptions::default().execution.sort_spill_reservation_bytes` is a good
guard, since it fails if DataFusion changes the default and the hint's "10 MiB"
goes stale. The three `SORT_MEMORY_HINT.contains(...)` checks after it compare
the constant against pieces of itself, so they can only fail when someone edits
the hint, and then they have to be edited along with it. The full-message
`assert_eq!` in the loop below already pins the hint text. Could you drop those
three and keep the default check?
The PR description still says "Comet uses neither" and "Comet doesn't use
those two keys". Could you update those to match the new hint?
--
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]
