fix(physical-plan): CoalescePartitionsExec panic on wasm32-unknown-unknown - #24890
fix(physical-plan): CoalescePartitionsExec panic on wasm32-unknown-unknown#24890kentkwu wants to merge 4 commits into
Conversation
…known CoalescePartitionsExec's multi-input branch spawns a task per input partition via JoinSet::spawn, which needs a tokio reactor and panics on wasm32-unknown-unknown. Add a single-threaded path (target_partitions == 1) that drains inputs sequentially instead. Follow-up to apache#24275, which fixed the same class of panic in collect_partitioned.
yinli-systems
left a comment
There was a problem hiding this comment.
I reproduced the browser panic and reviewed the scheduling and regression-test behavior. I don't think this should merge as-is because the fallback changes native scheduling and can stop polling exchange-backed partitions concurrently; the test also relies on an output order the operator does not guarantee. The inline comments include a wasm-only cooperative alternative and the validation I ran.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24890 +/- ##
==========================================
- Coverage 81.63% 81.62% -0.01%
==========================================
Files 1123 1123
Lines 409546 409558 +12
Branches 409546 409558 +12
==========================================
+ Hits 334319 334321 +2
- Misses 55591 55601 +10
Partials 19636 19636 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Per review, replace the target_partitions == 1 fast path with a wasm-cfg'd select_all branch. Preserves cooperative polling of all input partitions, avoids the deadlock risk sequential try_flatten introduced for exchange-backed inputs, and leaves native scheduling untouched. Also switches the wasmtest to assert_batches_sorted_eq!.
|
Thanks for the review @yinli-systems. I agree with the recommended changes and applied them |
yinli-systems
left a comment
There was a problem hiding this comment.
Thanks for addressing both points. The wasm-only select_all path preserves native scheduling and cooperative polling across input partitions, and the sorted assertion now matches the operator contract. I reviewed the updated diff and the full CI suite is green.
Which issue does this PR close?
UNION ALLpanics onwasm32-unknown-unknown#24886Rationale for this change
Multi-partition queries panic on
wasm32-unknown-unknownwhen they funnel throughCoalescePartitionsExec— most commonlyUNION ALL, but any plan whose intermediate results have more than one partition (e.g. a multi-file external table scan) trips this. The operator's multi-input branch spawns viaJoinSet::spawn, which requires a tokio reactor that isn't installed underwasm-bindgen-futures:This is a follow-up to #24275, which fixed the same class of panic in
collect_partitioned.What changes are included in this PR?
This PR adds a single-threaded path to
CoalescePartitionsExec::execute: when the session'starget_partitionsequals1, the operator drains its input partitions sequentially instead of spawning a task per partition.Behavior when
target_partitions > 1is unchanged. Since the native default isnum_cpus::get(), only callers who have explicitly opted out of parallelism take the new path.Are these changes tested?
Yes.
test_union_allregression indatafusion/wasmtestthat runs under bothwasm-pack test --chromeandtokio::test.cargo fmt --checkandcargo clippy -- -D warningsboth run clean.Are there any user-facing changes?
There are no API changes. On
wasm32-unknown-unknown, multi-partition queries no longer panic when the session hastarget_partitions = 1. Native callers running with the defaulttarget_partitions > 1are unaffected.