Preserve input order for grouped window evaluation - #23466
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesMultirank streaming Multirank window ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf_polars/tests/streaming/test_spmd.py`:
- Around line 597-602: Strengthen the test around local_result in the SPMD
collection flow by asserting each rank owns the expected rows in the expected
local order before calling allgather_polars_dataframe. Add rank-aware ownership
checks using the test’s existing distributed context, then retain the global
sorted assertion for the gathered result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2318405c-4ef3-4cb4-b28c-26ad24613dc7
📒 Files selected for processing (3)
python/cudf_polars/cudf_polars/streaming/actor_graph/over.pypython/cudf_polars/cudf_polars/streaming/select.pypython/cudf_polars/tests/streaming/test_spmd.py
💤 Files with no reviewable changes (1)
- python/cudf_polars/cudf_polars/streaming/select.py
|
/merge |
Follow-up to #23466 That PR introduced support for input-order-sensitive `over(...)` expressions without an explicit `order_by` argument. However, there is a bug: We do not properly sort by the original local row-position (only the rank and chunk index). Since the hash-partitioning of the forward shuffle is not technically "stable", we need to include this local row position in the forward sort operation as well. Authors: - Richard (Rick) Zamora (https://github.com/rjzamora) Approvers: - Lawrence Mitchell (https://github.com/wence-) URL: #23524
Description
Follow up to #23451
Fully addresses item (7) of #22032
Adds the necessary per-group sort operation to preserve input row order before evaluating input-order-sensitive
over(...)expressions without an explicitorder_byargument.This allows streaming execution of grouped
shiftandcum_sumwindows that rely on the incoming row order.Checklist