[Data] Add task-based shuffle v2 support for sort - #64875
Conversation
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
|
the microcheck error is unrelated to this PR |
There was a problem hiding this comment.
Code Review
This pull request introduces the task-based shuffle v2 path for sorting, implementing SortShuffleMapOp with online range-boundary sampling and integrating it into the physical planner. It also updates ShuffleReduceOp to optionally preserve partition order and modifies the shuffle map task to handle compatible but non-identical schemas during concatenation. The review feedback highlights two important improvement opportunities: dynamically scaling the fallback tuple in _set_boundaries to support multi-column sort keys, and optimizing table concatenation in _shuffle_map_task by attempting a fast-path pa.concat_tables before falling back to type promotion.
Signed-off-by: Jie-Kai Chang <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
|
On second thought, I think we should sample all the blocks or we might get unexpected data skews. Could you change the pr to sample all blocks before doing range partitions? |
Signed-off-by: 400Ping <jiekaichang@apache.org> # Conflicts: # python/ray/data/_internal/execution/operators/shuffle_operators/shuffle_reduce_operator.py # python/ray/data/_internal/logical/rules/operator_fusion.py # python/ray/data/_internal/planner/plan_all_to_all_op.py
…sort-shuffle-v2 Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
|
Hey @owenowenisme please take a look when you have time. Sorry for the late fix, I was at Beijing giving a talk in Community Over Code Asia. |
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
There was a problem hiding this comment.
Have we thought about a separate sampling op instead of putting it inside of the same operator?
This could make sampling naturally support pipelining with upstream ops, get rid of a lot of things (e.g. max inflight sampling, _finish_map_inputs ) and fused with upstream map op if we implement some sampling op with the same map operator.
But this is just my hunch, can you dig into that direction and see if this is possible?
Also when you're doing this, could you survey how other frameworks sample input blocks before partitioning ?
Signed-off-by: 400Ping <jiekaichang@apache.org>
Thanks, I dug into the separate sampling operator direction. A separate zero-copy pass-through
This would separate sampling from range partitioning, remove the custom There are two limitations that I found: First, the current Second, sort v1 calculates: When the first input block arrives, the final number of input blocks may still This matches the general structure used by other frameworks:
Ray Data cannot generally recompute an arbitrary upstream UDF safely, so the Would you be comfortable with changing the sampling implementation to a For Ref: |
|
My take is choose the way that integrate with current the best, we don't have to follow everything v1 has, so even if you are only sampling a fixed 20 rows per block I think it's okay.
About this I think we can have a custom op if map operator cannot do that. And we can disable fusing for this pr and figure ways to enable that as followup |
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: Jie-Kai Chang <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
|
The microcheck error is unrelated to this PR. |
|
@owenowenisme please take a look at this PR when you have time. |
| return boundaries | ||
|
|
||
| @staticmethod | ||
| def _uninitialized_partition_fn(block): |
There was a problem hiding this comment.
Not blocker but why we have both this and ensure_boundaries?
There was a problem hiding this comment.
ShuffleMapOp needs a picklable partition_fn at construction, but sampled boundaries only exist after SortSamplingOp has seen every block. So the constructor gets a fail-loud placeholder and _ensure_boundaries swaps in the real one on the first input.
Signed-off-by: 400Ping <jiekaichang@apache.org>
Signed-off-by: 400Ping <jiekaichang@apache.org>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 5cede2f. Configure here.
Signed-off-by: 400Ping <jiekaichang@apache.org>

Description
This PR adds task-based shuffle v2 support for
Dataset.sort().Local benchmark
Benchmark:
Environment:
Raw runtimes:
Additional information