Repository navigation
Conversation
coderfender
left a comment
There was a problem hiding this comment.
Looks good. I like that the routing test checks against DataFusion's BatchPartitioner. Since the end-to-end tests run without sockets, could we add one standalone-cluster test in ballista/client/tests that submits a range-repartition plan with a split stage (e.g. max_partitions_per_task = 1) and checks the output is globally sorted? A rough perf number for range vs hash routing would be nice too, but not blocking.
…ing it The static planner kept only hash repartitions as stage boundaries and dropped every other RepartitionExec. That is harmless for round-robin, which only spreads rows, but a range repartition is part of what a plan means. A plan that range-partitions and then sorts each partition (a distributed global sort) came back as partitions that were each sorted but not range-disjoint, with no error. A RepartitionExec with Partitioning::Range is now a stage boundary. The producer stage ends in a SortShuffleWriterExec that routes rows with DataFusion's own RangeExpr (the function RepartitionExec applies), so the consumer's partition i reads exactly range i. The writer shares everything after routing with the hash shuffle: buffering, spilling, the consolidated file and its index. The range travels in a new field of SortShuffleWriterExecNode; hash stages keep using the old one. Restricting a range-partitioned ShuffleReaderExec to a task's partitions now slices the split points along with them, instead of failing. A slice that does not ascend (a retried partition queued ahead of a later one) or is empty has no range partitioning to report, so it reports UnknownPartitioning rather than failing the task. A range repartition that must preserve its input order is rejected with an error: a shuffle cannot merge sorted inputs, and planning it anyway would return partitions that are range-disjoint but no longer sorted. RangeShuffleWriterExec and RangeShuffleReaderExec are not used. They need an ordered producer, while the repartition here is followed by the sort. Closes apache#2680
A range RepartitionExec in a submitted physical plan is now planned as a shuffle instead of being dropped, one that must preserve its order is refused, and SortShuffleWriterExecNode gains a field.
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.
- RowRouter takes the partitioning by value and no longer carries a partition count; the hash and range paths share one bucket reset. - Reword two comments that overstated things. RangeExpr shares BatchPartitioner's routing function (it is not "the function RepartitionExec applies"), and a restricted reader's partitioning is read by partitioned hash-join dynamic filters, which Ballista disables by default. - Drop overlapping tests and duplicated helpers, reuse roundtrip_plan in the serde tests, and shorten comments and the upgrade-guide entries.
94ccab6 to
fa5fe79
Compare
|
@coderfender Added in fa5fe79, under both the static and the adaptive planner, with Writing it turned up something useful. Since #2682 landed, submitted plans go through the adaptive planner by default, and it pins a range repartition as an exchange. On main that job fails with |
avantgardnerio
left a comment
There was a problem hiding this comment.
This seems complimentary to the existing AQE range partitioning, and necessary as you say since DataFusion plans now have Partitioning::Range, which Ballista needs to support.
|
Good question. Ballista never computes the split points for Comet. Spark does, in the driver, before the plan reaches Ballista. When Spark builds the shuffle for a range exchange ( Ballista is not called repeatedly. The sampling is a separate Spark job, and the Ballista job is submitted once. Comet Native Executors doesn't hand Ballista a range partitioning yet. Today the driver sends none of that, and the cluster side turns a range exchange into a coalesce, so the consuming stage reads it as one partition. That sorts correctly but runs in a single task. This PR is what lets us stop doing that, because the planner no longer drops This also means Ballista only needs to route against fixed split points here. It doesn't need |
…ss family The standalone scheduler binds localhost:0, which can resolve to ::1, while a push-staged standalone executor bound its gRPC listener on 127.0.0.1:0. The two address families can be handed the same port number, and the scheduler then dials the executor's registered localhost:<port> and reaches itself, failing launches with Unimplemented. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Which issue does this PR close?
Closes #2680.
Rationale for this change
The static planner kept only hash repartitions as stage boundaries and dropped every other
RepartitionExec. Dropping a round-robin repartition only costs parallelism, but a range repartition is part of what a plan means. A plan that range-partitions and then sorts each partition (a distributed global sort) came back as partitions that were each sorted but not range-disjoint, with no error. Comet Native Executors need this for Spark's range exchanges (apache/datafusion-comet#7288).What changes are included in this PR?
RepartitionExecwithPartitioning::Rangeis a stage boundary, like a hash one, so partitioniof the consuming stage reads exactly rangei. Round-robin repartitions are still dropped. A range repartition that must preserve its input order (preserve_order()) returns an error: a shuffle cannot merge sorted inputs, and planning it anyway would return partitions that are range-disjoint but no longer sorted.SortShuffleWriterExecacceptsPartitioning::Rangeand routes rows with DataFusion'sRangeExpr, which shares its routing function with theBatchPartitionerbehindRepartitionExec, so the shuffle puts every row where the in-process operator would have. Everything after routing (buffering, spilling, the consolidated file and its index) is shared with the hash shuffle.ShuffleReaderExecwith a range partitioning slices its split points along with its partitions, replacing the TODO and the error intask_builder.rs. A slice that does not ascend (a retried partition queued ahead of a later one) or is empty reportsUnknownPartitioningrather than failing the task: no split points describe it, andUnknownPartitioningclaims nothing about which rows each partition holds. (DataFusion's partitioned hash-join dynamic filters do read the probe side's reported partitioning, but Ballista turns dynamic filter pushdown off by default.)partitioningfield onSortShuffleWriterExecNodecarries the range partitioning. Hash stages keep usingoutput_partitioning, so a hash plan is encoded as before.UnresolvedShuffleExecandShuffleReaderExecalready carried a generalPartitioning.Two things this PR deliberately does not do:
RangeShuffleWriterExec/RangeShuffleReaderExec, which the issue mentions.RangeShuffleWriterExecrequires an ordered producer (try_newrejects a child with no output ordering) and its reader merges sorted runs, while the issue's own example sorts after the repartition. The range edge reuses the sort-shuffle writer, as the hash edge does.PinSubmittedShufflesRulealready pins a range repartition as an exchange. Onmainthat job fails withunsupported shuffle output partitioning, because only hash stages get a writer. The writer change here makes it run, so the adaptive path is fixed too. The pipelined reader still refuses to restrict a range partitioning, and nothing plans pipelined jobs yet.RangeExprextracts aScalarValueper row, so range routing will be slower than hash routing on large inputs (the same cost as DataFusion's ownRepartitionExec). I have not benchmarked it.Tests. Besides unit tests for routing (with
BatchPartitioneras an oracle), serde and reader restriction,planner/range_shuffle_tests.rsplans a range-partition-then-sort and runs the stages for real, without sockets: it restricts each task's plan, ships it through the codec, and runs the writer and reader. It checks that the final partitions are range-disjoint, sorted, and hold every row exactly once, with one task per partition, with tasks owning several partitions, and with a slice out of key order. With the planner change reverted, those tests fail with the symptom in the issue. Two tests inballista/client/tests/physical_plan_submission.rssubmit the same shape to a standalone cluster withmax_partitions_per_task = 1, one per planner. The job's output partitions come back in the order their tasks finished, so they check every key exactly once, each partition sorted, and no two partitions' key ranges overlapping. The static one fails with overlapping partitions when the planner change is reverted, and the adaptive one fails withunsupported shuffle output partitioningwhen the writer change is reverted.Other commits. Besides the fix, three commits add entries to the 56.0.0 upgrade guide, move the new proto field to number 11 (see the overlap with #2723 below), and simplify the routing code and trim the tests and comments after a review pass. The last commit adds the standalone-cluster tests.
Overlap with other open PRs.
restrict_plan_to_partitionsa thirdresolverargument, and both PRs append tests totask_builder.rs. Whichever lands second has a conflict in that test module, and the calls torestrict_plan_to_partitionsin this PR's tests needNoneadded.SortShuffleWriterExecroutes rows, with aShuffleRoutingenum in place of the writer'sPartitioningfield. The two conflict in the writer, serde, the planner and the proto, and whichever lands second has to be reworked onto the other's routing; a range partitioning would fit as a thirdShuffleRoutingvariant. This PR numbers its new proto field 11 because feat: let hash shuffles place rows by a client-computed partition id #2723 uses 10 on the same message.Are there any user-facing changes?
No SQL-visible changes. A physical plan submitted with a range
RepartitionExecnow returns correct results instead of range-overlapping partitions, and a range repartition that must preserve its order is rejected with an error.The generated
SortShuffleWriterExecNodestruct gains a publicpartitioningfield, so code that builds it with a struct literal needspartitioning: None. The only such use in this repo is updated. Both changes are described in the 56.0.0 upgrade guide, and the PR carries theapi-changelabel for the struct field.