Support diff() inside over() in cudf-polars - #23497
Merged
Merged
Conversation
rjzamora
added
feature request
github-actions
Bot
added
Python
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughGrouped window expressions now support ChangesGrouped window diff
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cudf_polars/dsl/utils/aggregations.py`:
- Line 31: Update the affected union type annotations so None appears at the
end, using the existing symbols and preserving their current types:
python/cudf_polars/tests/expressions/test_rolling.py lines 306, 325, 346, 367,
398, and 438 require direct changes. The listed sites in
python/cudf_polars/cudf_polars/dsl/utils/aggregations.py lines 31-31,
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py lines 377-377,
python/cudf_polars/tests/expressions/test_rolling.py lines 477-489,
python/cudf_polars/cudf_polars/streaming/utils.py lines 133-133,
python/cudf_polars/tests/streaming/test_rolling.py lines 66-84, and
python/cudf_polars/tests/streaming/test_spmd.py lines 503-503 require no direct
change unless their annotations contain the same RUF036 pattern; ensure Ruff
formatting passes afterward.
In `@python/cudf_polars/tests/streaming/test_spmd.py`:
- Line 597: Extend the shared-group SPMD test cases around the existing diff
expression to include literal offsets n=2 and n=-1 alongside the default diff()
case. Keep the expected-result labels aligned with each added expression so the
test validates state transfer across partition boundaries for both supported
non-default offsets.
🪄 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: 6aea5f7d-8f82-4cca-b5ed-4de588f1a546
📒 Files selected for processing (6)
python/cudf_polars/cudf_polars/dsl/expressions/rolling.pypython/cudf_polars/cudf_polars/dsl/utils/aggregations.pypython/cudf_polars/cudf_polars/streaming/utils.pypython/cudf_polars/tests/expressions/test_rolling.pypython/cudf_polars/tests/streaming/test_rolling.pypython/cudf_polars/tests/streaming/test_spmd.py
|
|
||
| _WINDOW_ONLY_UNARY_FUNCTIONS = frozenset( | ||
| {"rank", "fill_null_with_strategy", "cum_sum", "shift", "shift_and_fill"} | ||
| {"rank", "fill_null_with_strategy", "cum_sum", "diff", "shift", "shift_and_fill"} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ruff check \
python/cudf_polars/cudf_polars/dsl/utils/aggregations.py \
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py \
python/cudf_polars/tests/expressions/test_rolling.py \
python/cudf_polars/cudf_polars/streaming/utils.py \
python/cudf_polars/tests/streaming/test_rolling.py \
python/cudf_polars/tests/streaming/test_spmd.py
ruff format --check \
python/cudf_polars/cudf_polars/dsl/utils/aggregations.py \
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py \
python/cudf_polars/tests/expressions/test_rolling.py \
python/cudf_polars/cudf_polars/streaming/utils.py \
python/cudf_polars/tests/streaming/test_rolling.py \
python/cudf_polars/tests/streaming/test_spmd.pyRepository: rapidsai/cudf
Length of output: 6062
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- changed-file summary ---'
git diff --stat
printf '%s\n' '--- Ruff configuration ---'
rg -n --hidden --glob 'pyproject.toml' --glob 'ruff.toml' --glob '.ruff.toml' \
'^\[tool\.ruff|select|extend-select|ignore|line-length|target-version' .
printf '%s\n' '--- cited locations ---'
for f in \
python/cudf_polars/cudf_polars/dsl/utils/aggregations.py \
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py \
python/cudf_polars/tests/expressions/test_rolling.py \
python/cudf_polars/cudf_polars/streaming/utils.py \
python/cudf_polars/tests/streaming/test_rolling.py \
python/cudf_polars/tests/streaming/test_spmd.py
do
echo "### $f"
case "$f" in
*/aggregations.py) sed -n '24,36p' "$f" ;;
*/expressions/rolling.py) sed -n '370,382p' "$f" ;;
*/tests/expressions/test_rolling.py) sed -n '470,495p' "$f" ;;
*/streaming/utils.py) sed -n '126,138p' "$f" ;;
*/tests/streaming/test_rolling.py) sed -n '60,88p' "$f" ;;
*/tests/streaming/test_spmd.py) sed -n '498,508p' "$f" ;;
esac
done
printf '%s\n' '--- Ruff check with independent status ---'
ruff check \
python/cudf_polars/cudf_polars/dsl/utils/aggregations.py \
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py \
python/cudf_polars/tests/expressions/test_rolling.py \
python/cudf_polars/cudf_polars/streaming/utils.py \
python/cudf_polars/tests/streaming/test_rolling.py \
python/cudf_polars/tests/streaming/test_spmd.py
check_status=$?
echo "ruff check exit status: $check_status"
printf '%s\n' '--- Ruff format check with independent status ---'
ruff format --check \
python/cudf_polars/cudf_polars/dsl/utils/aggregations.py \
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py \
python/cudf_polars/tests/expressions/test_rolling.py \
python/cudf_polars/cudf_polars/streaming/utils.py \
python/cudf_polars/tests/streaming/test_rolling.py \
python/cudf_polars/tests/streaming/test_spmd.py
format_status=$?
echo "ruff format --check exit status: $format_status"
exit 0Repository: rapidsai/cudf
Length of output: 16136
Fix the six RUF036 errors in python/cudf_polars/tests/expressions/test_rolling.py at lines 306, 325, 346, 367, 398, and 438. Move None to the end of each union, such as list[str | pl.Expr] | None. Ruff formatting passes for all six files.
📍 Affects 6 files
python/cudf_polars/cudf_polars/dsl/utils/aggregations.py#L31-L31(this comment)python/cudf_polars/cudf_polars/dsl/expressions/rolling.py#L377-L377python/cudf_polars/tests/expressions/test_rolling.py#L477-L489python/cudf_polars/cudf_polars/streaming/utils.py#L133-L133python/cudf_polars/tests/streaming/test_rolling.py#L66-L84python/cudf_polars/tests/streaming/test_spmd.py#L503-L503
🤖 Prompt for 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.
In `@python/cudf_polars/cudf_polars/dsl/utils/aggregations.py` at line 31, Update
the affected union type annotations so None appears at the end, using the
existing symbols and preserving their current types:
python/cudf_polars/tests/expressions/test_rolling.py lines 306, 325, 346, 367,
398, and 438 require direct changes. The listed sites in
python/cudf_polars/cudf_polars/dsl/utils/aggregations.py lines 31-31,
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py lines 377-377,
python/cudf_polars/tests/expressions/test_rolling.py lines 477-489,
python/cudf_polars/cudf_polars/streaming/utils.py lines 133-133,
python/cudf_polars/tests/streaming/test_rolling.py lines 66-84, and
python/cudf_polars/tests/streaming/test_spmd.py lines 503-503 require no direct
change unless their annotations contain the same RUF036 pattern; ensure Ruff
formatting passes afterward.
Source: Coding guidelines
mroeschke
approved these changes
Jul 31, 2026
| ): | ||
| shift_expr = ne.value | ||
| assert isinstance(shift_expr, expr.UnaryFunction) | ||
| if shift_expr.name == "diff": |
Contributor
There was a problem hiding this comment.
comment: If we wanted to support polars.Expr.pct_change with over, would that implementation go here?
xref https://github.com/rapidsai/cudf/pull/23225/changes for the pylibcudf APIs used to implement pct_change
Contributor
Author
There was a problem hiding this comment.
Yes, exactly. I'd expect pct_change().over(...) to fit here, I think. Instead of value - shifted, we would just want (value / shifted) - 1 for "pct_change".
rjzamora
added
5 - Ready to Merge
Contributor
Author
|
/merge |
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
4 participants
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.
Description
Adds cudf-polars support for
diff(...).over(...)in grouped window expressions.This reuses the existing grouped shift path to respect group boundaries, then applies the subtraction needed for
diff. Supports literalnwithnull_behavior="ignore"for both in-memory and streaming execution.Partially addresses #19934
Checklist