You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ump45nose (via GitHub)" <gi...@apache.org> on 2026/10/18 19:00:00 UTC
[PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #7104:
URL: https://github.com/apache/datafusion-comet/pull/7104#issuecomment-6271123075
> The only failing job, Verify TPC-DS Results, crashed while installing the Rust toolchain (rustup-init panicked) before any test ran. Could someone rerun it? I can't rerun jobs from the fork.
done
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
Posted by "ump45nose (via GitHub)" <gi...@apache.org>.
ump45nose commented on PR #7104:
URL: https://github.com/apache/datafusion-comet/pull/7104#issuecomment-6270976627
The only failing job, Verify TPC-DS Results, crashed while installing the Rust toolchain (rustup-init panicked) before any test ran. Could someone rerun it? I can't rerun jobs from the fork.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
Posted by "ump45nose (via GitHub)" <gi...@apache.org>.
ump45nose commented on PR #7104:
URL: https://github.com/apache/datafusion-comet/pull/7104#issuecomment-6409207047
Rebased onto current `main` now that #7098 has been merged (f889b17). The PR is back to the single sort OOM commit, unchanged from what was approved; there were no conflicts in the patch itself. `cargo fmt --check` and `cargo clippy -p datafusion-comet-jni-bridge --all-targets -D warnings` are clean, and `sort_oom_error_names_the_comet_config` passes locally.
(AI-assisted reply.)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #7104:
URL: https://github.com/apache/datafusion-comet/pull/7104
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
Posted by "ump45nose (via GitHub)" <gi...@apache.org>.
ump45nose commented on code in PR #7104:
URL: https://github.com/apache/datafusion-comet/pull/7104#discussion_r4326156002
##########
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. I dropped the three `SORT_MEMORY_HINT.contains` checks and kept the default `sort_spill_reservation_bytes` assertion in 0c8c753, and updated the PR description 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: point the external sort OOM error at Comet's memory settings [datafusion-comet]
Posted by "ump45nose (via GitHub)" <gi...@apache.org>.
ump45nose commented on PR #7104:
URL: https://github.com/apache/datafusion-comet/pull/7104#issuecomment-6365706340
Rebased onto the rebased #7098 (a6451b8) as b7e86e6. I squashed the follow-up commits into one, since their intermediate states no longer compiled against main's single-argument `classify`; the final diff is unchanged apart from that conflict resolution. `sort_oom_error_names_the_comet_config` and the spill-limit test pass locally, and clippy and rustfmt are clean. (AI-assisted reply.)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org