viirya commented on code in PR #22038:
URL: https://github.com/apache/datafusion/pull/22038#discussion_r3911340458
##########
datafusion/common/src/config.rs:
##########
@@ -1031,6 +1031,25 @@ config_namespace! {
/// Default: 128 MB
pub max_spill_file_size_bytes: ConfigNonZeroUsize, default =
non_zero_usize_default(128 * 1024 * 1024)
+ /// Enables the memory-limited fallback for `NestedLoopJoinExec` join
+ /// types that emit unmatched left rows in the final output (LEFT, LEFT
+ /// SEMI, LEFT ANTI, LEFT MARK, FULL) when the right side has multiple
+ /// partitions.
+ ///
+ /// This fallback coordinates per-chunk left state (visited bitmap and
+ /// probe-thread counter) across all right-side partitions, which
+ /// assumes every partition runs in the same process. Distributed
+ /// engines that execute each output partition as an independent task
+ /// (e.g. Ballista, datafusion-distributed) build a separate
coordinator
+ /// per task and poll only one partition, so the cross-partition
+ /// counter never reaches zero and the fallback would stall. Such
+ /// engines should set this to `false`: the coordinated fallback is
then
+ /// disabled for left-emitting multi-partition joins, which instead
fail
+ /// with a resource-exhaustion error under memory pressure rather than
+ /// deadlocking. Single-partition and non-left-emitting joins are
+ /// unaffected and always keep the fallback.
+ pub enable_nlj_coordinated_fallback: bool, default = true
Review Comment:
You're right that `constructible_struct_adds_field` is a genuine finding —
`config_namespace!` expands to a plain `pub struct` with no
`#[non_exhaustive]`, so `ExecutionOptions` is exhaustively constructible and
any new `pub` field breaks downstream struct literals. No argument there.
Where I'd like your read is on whether that makes this PR different from how
the project normally adds a config option, because a few things suggest it
doesn't:
- The `Check semver` workflow is advisory rather than a gate — its own
header says it "only runs cargo-semver-checks and uploads the result as an
artifact", with the comment posted by a companion workflow. It's currently
**passing** on this PR.
- There's recent precedent for exactly this change shape: #23651 added `pub
use_statistics_registry: bool` to a `config_namespace!` struct and merged.
- The bot note on this PR is dated 2026-07-19, i.e. against the pre-rebase
revision, so it may not reflect the current diff.
So my reading is that this is the same API-compatibility cost every
DataFusion config option carries, and that avoiding it here would make
`enable_nlj_coordinated_fallback` the one execution option not reachable
through `ExecutionOptions` / `SET` — which seems worse for users and for
maintenance than the breakage it avoids.
That said, "this is the project's normal practice" is a call for the
maintainers rather than something I want to assert unilaterally, so: do you
read the precedent differently, or is there a policy change since #23651 that
I've missed? If the view is that new config fields now need to wait for a major
version, I'm happy to hold this behind that — I'd just want it to apply to
config options generally rather than to this one.
One option if you want the improvement without the API question: drop the
flag entirely and always use the coordinated fallback. The trade-off is that
distributed engines then have no opt-out, and for them the shared probe-thread
counter never reaches zero, so the fallback stalls instead of failing fast.
That's why the flag exists, and it's why I'd rather keep it — but if you prefer
that shape, say so and I'll make the change.
--
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]
