Repository navigation
Conversation
andygrove
added a commit
to andygrove/datafusion-ballista
that referenced
this pull request
Oct 31, 2026
apache#2723 adds partition_id_routing to the same message as field 10, and two fields cannot share a number. 11 leaves 10 to it, and putting the field next to output_partitioning keeps it clear of the fields that PR appends at the end of the message.
A hash shuffle always assigns rows to partitions with DataFusion's hash, so it cannot match data that is already partitioned by another function, such as Spark's Murmur3 for bucketed tables. Add a partition-id mode to the sort-based shuffle writer. The plan below the writer computes each row's output partition into an integer column, and the writer sends the row to that partition and drops the column before writing. Output partition i holds exactly the rows whose id is i. An id that is null, negative or out of range fails the task. - ShuffleRouting selects between hashing and partition-id routing in SortShuffleWriterExec. try_new is unchanged; try_new_with_routing takes the routing, and the executor stamps the decoded writer rather than rebuilding it field by field, so the routing is carried over. - PartitionIdRepartitionExec is the marker a client places where a hash RepartitionExec would go. The static distributed planner turns it into a stage boundary ending in the new writer. It cannot be executed. - Both the marker and the writer's routing have proto messages and codec support (a oneof on the sort writer node, wire-compatible with the old field), so plans containing them can be submitted over gRPC. - The adaptive planner rejects the marker: it may coalesce or re-plan an exchange's partitions, which would break the placement. Closes apache#2681.
`losing_own_shuffle_output_restores_the_cache_reader` (apache#2633) names `dyn ExecutionPlan` but relied on the test module's import of it, which apache#2613 removed along with the test helper that was its only other user. Each passed on its own base, but together `ballista-scheduler`'s lib tests no longer compile on main.
andygrove
force-pushed
the
feat/2681-partition-id-shuffle
branch
from
October 31, 2026 16:05
63a7584 to
5c34a31
Compare
The conflicts all came from apache#2701 taking tag 20 of BallistaPhysicalPlanNode: - ballista.proto: the marker moves to tag 21. - serde/mod.rs: both sets of imports and codec arms are kept. - generated/ballista.rs: upstream's file plus this PR's messages. The tonic-prost-build 0.14.7 bump makes a rebuild add #[allow(clippy::all)] to every type; those lines are left out so the diff stays this PR's own.
coderfender
reviewed
Nov 3, 2026
coderfender
left a comment
Contributor
There was a problem hiding this comment.
Thanks for implementing this, Andy. A neat way to support Spark-bucketed data without teaching Ballista Spark's hash, and the strict handling of bad ids plus the wire-compat story are great. Two small nits:
- The writer's statistics drop the id column's
column_statistics, buttotal_byte_sizestill counts it. - In pipelined mode a plan with the marker fails with "PipelinedShuffleWriterExec only supports Hash partitioning, got: UnknownPartitioning(n)". A message naming
PartitionIdRepartitionExecwould be clearer. Fine as a follow-up since pipelined mode is experimental.
Count a partition-id shuffle's null counts through the column stats collector on the written columns only, take the dropped id column's bytes out of the writer's total_byte_size, and refuse the marker with a clear error in pipelined execution.
Member
Author
|
Both fixed in 52c427a. The writer's |
andygrove
added a commit
to andygrove/datafusion-ballista
that referenced
this pull request
Nov 10, 2026
apache#2723 adds partition_id_routing to the same message as field 10, and two fields cannot share a number. 11 leaves 10 to it, and putting the field next to output_partitioning keeps it clear of the fields that PR appends at the end of the message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #2681.
Rationale for this change
A hash shuffle always assigns rows to partitions with DataFusion's hash. That is consistent within a job, so joins and aggregates over shuffled inputs are correct, but it can't match data that is already partitioned by another function. Spark partitions by Murmur3, and bucketed tables and storage-partitioned joins depend on it. When one side of a join is a Spark-bucketed table read without a shuffle, the other side has to be shuffled with Spark's hash for matching rows to meet. Comet Native Executors (apache/datafusion-comet#7288) turn bucketing off today for this reason.
This implements the first option in the issue: the plan computes each row's partition into a column and the writer routes by it. A pluggable hasher would need every executor to have the hasher registered, and Ballista encodes the expressions in its shuffle nodes with DataFusion's default codec, so a client's own
PhysicalExprcouldn't be sent to executors anyway.What changes are included in this PR?
SortShuffleWriterExectakes aShuffleRouting:Hash(what it does today) orPartitionId { column, partition_count }. In partition-id mode, output partitioniholds exactly the rows whose id isi, and the id column is dropped before writing, so the shuffle's schema is the input's without it. Any integer type works. A null, negative or out-of-range id fails the task rather than being wrapped into some partition.try_newandshuffle_output_partitioning()are unchanged;try_new_with_routing,routing(),output_schema()andwith_work_dir()are new. The executor now stamps the decoded writer withwith_work_dir, as its pipelined arm already does, instead of rebuilding it field by field, so it can't drop the routing.PartitionIdRepartitionExecgoes where a hashRepartitionExecwould. It is only a marker: the static distributed planner turns it into a stage that ends in the new writer, andexecutereturns an error.create_shuffle_writer_with_confignow takes anOption<ShuffleRouting>for hash repartitions and the marker alike; the AQE adapter converts itsPartitioningwith a newTryFrom<Partitioning> for ShuffleRouting. Its output reportsUnknownPartitioning, since nothing downstream can assume rows are placed the way DataFusion would place them.PartitionIdRepartitionExecNode(tag 21 inBallistaPhysicalPlanNode, with the id column and partition count inline) andPartitionIdRoutingmessages, so plans containing either can be submitted over gRPC with the physical extension codec.SortShuffleWriterExecNodenow holds its routing as aoneof: the existingoutput_partitioning = 4or the newpartition_id_routing = 10. Aoneofmember is encoded like a plain field, so plans from older schedulers still decode. An executor that predates the new field sees neither routing and rejects the node, so the plan fails to launch instead of being hashed.Interaction with #2682
On
maintoday, submitted physical plans are always planned statically, so this works as is. #2682 sends them through the adaptive planner by default, which would reject the marker. Whichever of the two lands second needs to handle it. The options are keeping plans that containPartitionIdRepartitionExecon the static planner inTaskManager::submit_plan, or supporting the marker in the adaptive planner as an exchange that is never coalesced. The adapter now builds its writer from aShuffleRouting, so the exchange would only need to carry one, and the pinning and coalesce guard in #2682 look like the right base.Overlap with #2722
#2722 (plan a range
RepartitionExecas a shuffle stage) also generalises howSortShuffleWriterExecroutes rows: it keeps aPartitioning(HashorRange), builds aRowRouterper input partition, and addspartitioning = 11toSortShuffleWriterExecNode. This PR makes the routing an enum,ShuffleRouting, carried by aoneof. Neither depends on the other, but they conflict inballista.proto,sort_shuffle/writer.rs,serde/mod.rs,planner.rsandshuffle_writer.rs, so whichever merges second has to reconcile them. Range would most likely become a thirdShuffleRoutingvariant, withpartitioning = 11joining theroutingoneof.A second, unrelated commit
fix(scheduler): import ExecutionPlan in the execution graph testsis one line, and it is a compile error that is onmainright now:losing_own_shuffle_output_restores_the_cache_reader(#2633) usesdyn ExecutionPlanbut relied on an import that #2613 removed along with the helper that was its only other user.ballista-scheduler's lib tests don't build without it (reproduced on a clean export ofapache/mainat 2342897), and this PR adds tests there. It is its own commit so it can be dropped if a fix lands first; #2721 carries the same fix as its first commit, so whichever of the two merges first makes the other's a no-op.Tests
ballista/scheduler/tests/partition_id_shuffle.rs: plans a marker query, runs the writer stage through the executor engine'screate_query_stage_execon real rows, resolves the reader, and checks that every partition holds exactly the rows the client placed there and that the column stats cover only the written columns. It needs no sockets.Ran locally:
cargo testfor core, scheduler and executor (including the TPC-H plan stability snapshots, which are unchanged),cargo fmt,cargo clippy --all-targets --all-features -- -D warningson those crates, rustdoc with-D warnings, a workspacecargo check --all-targets, and the pinned prettier on the docs page.Not run locally: tests that bind a local socket (the cluster tests in
ballista/client/testsand a handful of unit tests), because my environment blocks those binds. They are left to CI.Are there any user-facing changes?
New public API:
PartitionIdRepartitionExec,ShuffleRouting(withoutput_schemaandTryFrom<Partitioning>), andSortShuffleWriterExec::{try_new_with_routing, routing, with_work_dir}, documented in the tuning guide. Hash shuffles behave and display exactly as before, and the wire encoding of existing plans is unchanged. The one change to an existing public item is in generated code:protobuf::SortShuffleWriterExecNode.output_partitioningis now a member of theroutingoneof, which matters only to code that builds or reads that message directly.