Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new TopN logical operator and an associated rewrite rule to optimize Sort + Limit operations into a more efficient TopN execution path. The implementation includes a local candidate reduction phase followed by a global merge, supported by a configurable budget to manage resource usage. My feedback suggests refactoring the rewrite function in top_n_rewrite.py to check for non-positive limits earlier, which improves code clarity and prevents invalid TopN operator creation.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 230bff4. Configure here.
Add a default-off Shuffle V2 Top-N rewrite with local candidate reduction, a guarded final merge, and focused logical, operator, and execution tests. Signed-off-by: viiccwen <vicwen@apache.org>
Propagate DataContext to the final merge task, preserve non-positive Limit plans, and defer the Polars rewrite until null ordering is aligned by ray-project#66925. Add regression coverage for these cases. Signed-off-by: viiccwen <vicwen@apache.org>
Add Bazel py_test targets for the Top-N rewrite, operator, and execution suites so CI discovers and runs them. Signed-off-by: viiccwen <vicwen@apache.org>
Add the standard direct pytest entry point required by Ray's Bazel py_test format checker. Signed-off-by: viiccwen <vicwen@apache.org>
b0a574d to
edfd31d
Compare
Signed-off-by: viiccwen <vicwen@apache.org>
owenowenisme
left a comment
There was a problem hiding this comment.
High level feedbacks:
- I'd suggest implementing a new operator based on PhysicalOperator (maybe on top of MapOperator, which might save a lot of work), since AllToAllOperator will be deprecated soon, once all our shuffles move to shuffle v2 and v1 is deprecated.
- For
top_n_rewrite_max_limit, I suggest we survey how other engines set this number, or sweep it ourselves. I'd also suggest defaultingtop_n_rewrite_enabledto true. - For the final TopN merge, we could probably fetch the inputs in parts and maintain a top-N heap, instead of fetching all at once. With a native operator (1), this could even be done incrementally as candidates arrive.
- Check
pyarrow.compute.select_k_unstableto avoid sorting the whole block (though we probably need to check what problems the unstable ordering could cause). - I think
LocalTopNshould be able to fuse with upstream.
|
I ran some benchmarks on this branch to get memory numbers and to see how k and the number of blocks affect it. Setup: one node, 32 CPUs, Ray nightly a705161 + this PR. Both sides use SHUFFLE_V2 and only
A few things I noticed:
|
Hello @owenowenisme. Sorry for lately replying and appreciate ur suggestion!
Our benchmarks also show that Top-N remains beneficial at k=100,000. Since Ray's candidate volume depends on k, block count, and row width, I think a candidate-aware budget would be more appropriate than a fixed threshold alone. I'll run additional end-to-end benchmarks before deciding on
Thanks again! Let me know what you think about the PR scope, especially points 1 and 4. Edit: After discussing, we should put point 1, 3, 4, 5 in this PR. |
Add an exact BlockAccessor Top-N primitive for Arrow and Pandas, with correctness-preserving fallbacks for nulls, NaNs, mixed directions, and unsupported kernels. Reuse it in the local and final V2 Top-N stages. Signed-off-by: viiccwen <vicwen@apache.org>
Track merge tasks, resource usage, output queues, and cancellation through the physical operator lifecycle. Preserve candidate budgets, empty schemas, and upstream object-store backpressure exemptions, with focused lifecycle and remote-option regressions. Signed-off-by: viiccwen <vicwen@apache.org>
Describe the local Top-N stage as a map for fusion compatibility and validate sort keys after upstream transforms. Cover fused read/map plans, schema-changing maps, and incompatible compute or remote arguments while keeping the final merge separate. Signed-off-by: viiccwen <vicwen@apache.org>
Parameterize the existing multi-key execution test over zero, minimum, partial, exact-size, and oversized limits on a ten-row dataset. Assert positive limits use the Top-N execution path. Signed-off-by: viiccwen <vicwen@apache.org>
bc7a37d to
96b0456
Compare

Description
This PR adds a Top-N execution path for eligible Ray Data
sort(...).limit(k)plans. WithDataContext.shuffle_strategy = ShuffleStrategy.SHUFFLE_V2and the privatetop_n_rewrite_enabledconfig enabled, eligible plans are rewritten as:TopNRewriteRulerewritesLimit(k, Sort(...))to a logicalTopN. It runs afterLimitPushdownRuleandCombineShuffles, so it also handles eligible plans where row-preserving operators originally separate Sort and Limit, and collapses consecutive limits to the smaller value. Sorts with user-provided boundaries are left unchanged.LocalTopNreusesMapOperator. It sorts each input block and retains at mostkrows before the final merge, avoiding sort sampling and the full range shuffle. The map is not fused into the final merge.TopNMergeOperatorcollects the locally sorted candidates, normalizes mixed Arrow and Pandas block types, merges the candidates, and returns one globally sorted Top-N block. Empty Arrow and Pandas inputs retain their schema.This path is private and default-off. It only supports
SHUFFLE_V2; legacy sort shuffle is unchanged.Benchmark
The workload is
range -> map_batches(randomized key) -> sort(["key", "row_id"]) -> limit(k) -> materialize.int64int64int64int64int64+ 128B payloadRelated Issues
Related to #66831.
Additional information
Follow-up work (Important)
AllToAllOperatorwrapper with a native asynchronousPhysicalOperatorlifecycle, including task tracking and resource accounting.