Support pl.Expr.reverse, reverse=True for Polars cummulative expressions - #23164
Conversation
github-actions
Bot
added
Python
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py (1)
551-562: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFlip null precedence for reverse grouped cumulative scans.
reverse=Trueflips the order-by direction and row-id tie-breaker, but_build_window_order_indexstill uses the forward null ordering. That makes.over(order_by=...)cumulative scans return wrong results whenever the order-by column contains nulls.Proposed fix
nulls.append( plc.types.NullOrder.AFTER - if ob_desc ^ ob_nulls_last + if ob_desc ^ ob_nulls_last ^ reverse else plc.types.NullOrder.BEFORE )🤖 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/expressions/rolling.py` around lines 551 - 562, The reverse grouped cumulative scan path is still using the forward null precedence when building the window ordering, so `.over(order_by=...)` can misorder rows with nulls. Update `_build_window_order_index` in `rolling.py` so the null ordering is flipped consistently with `reverse` alongside the order-by direction and row-id tie-breaker, using the existing `order_by_col`, `orders`, and `nulls` handling.Source: Coding guidelines
🧹 Nitpick comments (2)
python/pylibcudf/tests/test_copying.py (1)
458-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd edge-case coverage for
reverse.Both new tests rely on the shared
target_column/source_tablefixtures, which only exercise 6-element, all-valid data. Consider adding cases for an empty column/table, a single-element column, and a column/table containing nulls to verifyreversepreserves null positions/validity correctly.As per coding guidelines,
python/**/test_*.py: "Ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types) and do not depend on external datasets."🤖 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/pylibcudf/tests/test_copying.py` around lines 458 - 472, The new reverse tests in test_copying.py only cover the shared 6-element all-valid fixtures, so expand coverage in test_reverse_column and test_reverse_table to include empty, single-element, and null-containing cases. Add explicit assertions using plc.copying.reverse on these edge inputs and verify the expected PyArrow results preserve ordering, null positions, and validity metadata correctly.Source: Coding guidelines
python/cudf_polars/tests/expressions/test_agg.py (1)
116-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing edge-case and windowed (
.over()) coverage for reverse cumulative aggregations.
test_cum_agg_reversecovers empty and mixed-null flat columns but not all-null or single-element columns. More importantly, it doesn't cover.over(...)grouped/window reverse cum_sum, which is a substantial new code path inrolling.py::GroupedWindow(order-by direction/null flipping, row_id tie-break flipping). Adding a case withorder_bycontaining nulls plusreverse=Truewould have caught the null-placement issue flagged inrolling.py.As per coding guidelines,
python/**/test_*.py: "Ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types) and do not depend on external datasets."🤖 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/tests/expressions/test_agg.py` around lines 116 - 129, `test_cum_agg_reverse` only exercises flat reverse cumulative aggregations, so extend it with missing edge cases (single-element and all-null inputs) and add a `.over(...)` windowed reverse cumulative aggregation scenario. Use `pl.col("a")` with `reverse=True` in combination with an `order_by` containing nulls to cover the `GroupedWindow` path in rolling/window logic and validate the null-placement and row-id tie-breaking behavior. Ensure the new cases stay within `test_cum_agg_reverse` or a nearby dedicated test so the coverage clearly targets the reverse cumulative aggregation code path.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@python/cudf_polars/cudf_polars/dsl/expressions/rolling.py`:
- Around line 551-562: The reverse grouped cumulative scan path is still using
the forward null precedence when building the window ordering, so
`.over(order_by=...)` can misorder rows with nulls. Update
`_build_window_order_index` in `rolling.py` so the null ordering is flipped
consistently with `reverse` alongside the order-by direction and row-id
tie-breaker, using the existing `order_by_col`, `orders`, and `nulls` handling.
---
Nitpick comments:
In `@python/cudf_polars/tests/expressions/test_agg.py`:
- Around line 116-129: `test_cum_agg_reverse` only exercises flat reverse
cumulative aggregations, so extend it with missing edge cases (single-element
and all-null inputs) and add a `.over(...)` windowed reverse cumulative
aggregation scenario. Use `pl.col("a")` with `reverse=True` in combination with
an `order_by` containing nulls to cover the `GroupedWindow` path in
rolling/window logic and validate the null-placement and row-id tie-breaking
behavior. Ensure the new cases stay within `test_cum_agg_reverse` or a nearby
dedicated test so the coverage clearly targets the reverse cumulative
aggregation code path.
In `@python/pylibcudf/tests/test_copying.py`:
- Around line 458-472: The new reverse tests in test_copying.py only cover the
shared 6-element all-valid fixtures, so expand coverage in test_reverse_column
and test_reverse_table to include empty, single-element, and null-containing
cases. Add explicit assertions using plc.copying.reverse on these edge inputs
and verify the expected PyArrow results preserve ordering, null positions, and
validity metadata correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2156cd25-ab94-4fcb-a663-daad6d3d4f3c
📒 Files selected for processing (8)
python/cudf_polars/cudf_polars/dsl/expressions/rolling.pypython/cudf_polars/cudf_polars/dsl/expressions/unary.pypython/cudf_polars/tests/expressions/test_agg.pypython/pylibcudf/pylibcudf/copying.pxdpython/pylibcudf/pylibcudf/copying.pyipython/pylibcudf/pylibcudf/copying.pyxpython/pylibcudf/pylibcudf/libcudf/copying.pxdpython/pylibcudf/tests/test_copying.py
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/expressions/test_agg.py`:
- Around line 116-149: Add a single-element edge case to the aggregation tests
so they cover the missing boundary condition. Update test_product and/or
test_cum_count in test_agg.py by extending the existing parametrized data sets
with a one-item input and verify the existing assert_gpu_result_equal coverage
still passes for the relevant expression methods (pl.col("a").product() and
pl.col("a").cum_count()).
🪄 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: 07f34194-4750-4f14-b632-e3aa6c840d2d
📒 Files selected for processing (2)
python/cudf_polars/cudf_polars/dsl/expressions/unary.pypython/cudf_polars/tests/expressions/test_agg.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cudf_polars/cudf_polars/dsl/expressions/unary.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/expressions/test_agg.py`:
- Around line 116-149: Add a single-element edge case to the aggregation tests
so they cover the missing boundary condition. Update test_product and/or
test_cum_count in test_agg.py by extending the existing parametrized data sets
with a one-item input and verify the existing assert_gpu_result_equal coverage
still passes for the relevant expression methods (pl.col("a").product() and
pl.col("a").cum_count()).
🪄 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: 07f34194-4750-4f14-b632-e3aa6c840d2d
📒 Files selected for processing (2)
python/cudf_polars/cudf_polars/dsl/expressions/unary.pypython/cudf_polars/tests/expressions/test_agg.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cudf_polars/cudf_polars/dsl/expressions/unary.py
🛑 Comments failed to post (1)
python/cudf_polars/tests/expressions/test_agg.py (1)
116-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Minor: missing single-element edge case.
test_productandtest_cum_countcover empty and all-null inputs but omit a single-element dataset.As per path instructions for
python/**/test_*.py: "Ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types)".🤖 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/tests/expressions/test_agg.py` around lines 116 - 149, Add a single-element edge case to the aggregation tests so they cover the missing boundary condition. Update test_product and/or test_cum_count in test_agg.py by extending the existing parametrized data sets with a one-item input and verify the existing assert_gpu_result_equal coverage still passes for the relevant expression methods (pl.col("a").product() and pl.col("a").cum_count()).Source: Path instructions
| if reverse: | ||
| # A reverse cumulative aggregation is a forward one over | ||
| # the reversed column, reversed back into place. | ||
| counts = plc.copying.reverse(counts, stream=df.stream) | ||
| result = plc.reduce.scan( | ||
| counts, | ||
| plc.aggregation.sum(), | ||
| plc.reduce.ScanType.INCLUSIVE, | ||
| stream=df.stream, | ||
| ) | ||
| if reverse: | ||
| result = plc.copying.reverse(result, stream=df.stream) | ||
| return Column(result, dtype=self.dtype) |
There was a problem hiding this comment.
CUB and thrust definitely support scans over iterators, so I think we should try and instead offer an option to use a reverse iterator in libcudf scans.
Rather than having to copy everything twice.
There was a problem hiding this comment.
Sure thing. Opened #23208 and linked here for a reverse scan in libcudf
mroeschke
changed the title
reverse=True for Polars cummulative expressionspl.Expr.reverse, reverse=True for Polars cummulative expressions
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesReverse copying and cumulative evaluation
Estimated code review effort: 4 (Complex) | ~45 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py (3)
675-686: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReverse null precedence with the sort direction.
reverseflips the order-by and row-id directions but leaves null placement unchanged. A reverse scan must use the exact inverse ordering; for example, ascending/nulls-last becomes descending/nulls-first.Proposed fix
nulls.append( plc.types.NullOrder.AFTER - if ob_desc ^ ob_nulls_last + if ob_desc ^ ob_nulls_last ^ reverse else plc.types.NullOrder.BEFORE )🤖 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/expressions/rolling.py` around lines 675 - 686, Update the null-order calculation in the order_by_col handling of the rolling expression so reverse inverts null precedence along with sort direction. Ensure the reverse path produces the exact inverse ordering, such that ascending/nulls-last becomes descending/nulls-first, while preserving existing behavior when reverse is false.
1080-1116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the selected reverse direction to the grouped scan.
is_reverseis never passed to_grouped_window_scan_setup, so reverse cum_sum uses forward ordering. Also, wrappedfill_null_with_strategy(cum_sum(...))reads the fill strategy string as the reverse flag, making it always truthy.Proposed fix
cum_reverse = [] for ne in cum_named: - assert isinstance(ne.value, expr.UnaryFunction) - cum_reverse.append(bool(ne.value.options[0])) + cumulative = ne.value + assert isinstance(cumulative, expr.UnaryFunction) + if cumulative.name == "fill_null_with_strategy": + (cumulative,) = cumulative.children + assert isinstance(cumulative, expr.UnaryFunction) + cum_reverse.append(bool(cumulative.options[0])) ... grouper=grouper, + reverse=is_reverse, stream=df.stream, require_sorted_groups=has_fill,🤖 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/expressions/rolling.py` around lines 1080 - 1116, Update the grouped scan setup inside the `for is_reverse in (False, True)` loop to pass the selected `is_reverse` direction to `_grouped_window_scan_setup` rather than deriving it from the wrapped expression options. Ensure the reverse flag is read from the outer cumulative reverse classification so `fill_null_with_strategy(cum_sum(...))` does not interpret its fill strategy string as a boolean.
563-570: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPass
df.streaminto bothGroupBycalls.local_grouperdoesn’t carry a bound stream, soscan()andreplace_nulls()can fall back to a different stream than the surrounding work.🤖 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/expressions/rolling.py` around lines 563 - 570, Update both GroupBy constructions in the rolling expression flow to receive the surrounding df.stream explicitly. Ensure the local_grouper used by scan() and replace_nulls() is bound to that same stream, preserving stream consistency for all grouped operations.Source: Coding guidelines
🧹 Nitpick comments (1)
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py (1)
214-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported
FixedSizeRollingWindowAPI.It is in
__all__, but its docs omit constructor parameters, supported aggregations, and evaluation/return behavior. As per coding guidelines, “Ensure all public API methods have complete docstrings documenting parameters, return values, and behavior” and “Ensure new public APIs are added to documentation.”🤖 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/expressions/rolling.py` around lines 214 - 281, Expand the class docstring for exported FixedSizeRollingWindow to document all constructor parameters, the supported aggregation names, and its evaluation/return behavior, including the fixed integer-based window semantics. Keep the documentation aligned with the actual constructor, _aggregations mapping, and expression behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@python/cudf_polars/cudf_polars/dsl/expressions/rolling.py`:
- Around line 675-686: Update the null-order calculation in the order_by_col
handling of the rolling expression so reverse inverts null precedence along with
sort direction. Ensure the reverse path produces the exact inverse ordering,
such that ascending/nulls-last becomes descending/nulls-first, while preserving
existing behavior when reverse is false.
- Around line 1080-1116: Update the grouped scan setup inside the `for
is_reverse in (False, True)` loop to pass the selected `is_reverse` direction to
`_grouped_window_scan_setup` rather than deriving it from the wrapped expression
options. Ensure the reverse flag is read from the outer cumulative reverse
classification so `fill_null_with_strategy(cum_sum(...))` does not interpret its
fill strategy string as a boolean.
- Around line 563-570: Update both GroupBy constructions in the rolling
expression flow to receive the surrounding df.stream explicitly. Ensure the
local_grouper used by scan() and replace_nulls() is bound to that same stream,
preserving stream consistency for all grouped operations.
---
Nitpick comments:
In `@python/cudf_polars/cudf_polars/dsl/expressions/rolling.py`:
- Around line 214-281: Expand the class docstring for exported
FixedSizeRollingWindow to document all constructor parameters, the supported
aggregation names, and its evaluation/return behavior, including the fixed
integer-based window semantics. Keep the documentation aligned with the actual
constructor, _aggregations mapping, and expression behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9065c2a8-a00b-47ae-a204-84866246f0a8
📒 Files selected for processing (3)
python/cudf_polars/cudf_polars/dsl/expressions/rolling.pypython/cudf_polars/cudf_polars/dsl/expressions/unary.pypython/cudf_polars/cudf_polars/testing/inject_gpu_engine.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cudf_polars/cudf_polars/testing/inject_gpu_engine.py
- python/cudf_polars/cudf_polars/dsl/expressions/unary.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py (2)
675-686: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReverse the null precedence together with the sort direction.
When
reverse=True, theorder_bydirection is flipped at Lines [678-680], but null precedence remains unchanged. Reversing an ordered sequence must also move nulls from before to after (or vice versa); otherwise rows with nullorder_byvalues are scanned in the wrong order.Proposed fix
nulls.append( plc.types.NullOrder.AFTER - if ob_desc ^ ob_nulls_last + if ob_desc ^ ob_nulls_last ^ reverse else plc.types.NullOrder.BEFORE )🤖 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/expressions/rolling.py` around lines 675 - 686, Update the null-order selection in the order_by handling of the rolling expression so reverse=True flips null precedence along with sort direction. Adjust the condition used for nulls.append, preserving the existing ob_desc and ob_nulls_last behavior when reverse is false.
1080-1089: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRead the nested
cum_sumreverse flag for filled cumulative expressions.For
fill_null_with_strategy(cum_sum(...)),ne.value.options[0]is the fill strategy, not the nestedcum_sumreverse flag. Since_split_named_exprintentionally places this wrapper in thecum_sumset, non-empty strategy strings are coerced toTrue, incorrectly routing every wrapped cumulative expression through the reverse scan.Unwrap the fill expression before reading
options[0], and add regression coverage for both forward and reverse filled cumulative sums.Proposed fix
cum_reverse = [] for ne in cum_named: - assert isinstance(ne.value, expr.UnaryFunction) - cum_reverse.append(bool(ne.value.options[0])) + value = ne.value + assert isinstance(value, expr.UnaryFunction) + if value.name == "fill_null_with_strategy": + value = value.children[0] + assert isinstance(value, expr.UnaryFunction) + cum_reverse.append(bool(value.options[0]))🤖 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/expressions/rolling.py` around lines 1080 - 1089, Update the cum_reverse extraction in the cumulative-expression grouping logic to unwrap fill_null_with_strategy expressions and read the nested cum_sum reverse flag rather than the fill strategy at ne.value.options[0]. Preserve direct cum_sum handling, ensure forward and reverse filled cumulative expressions route to their respective scans, and add regression coverage for both cases.
🤖 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.
Outside diff comments:
In `@python/cudf_polars/cudf_polars/dsl/expressions/rolling.py`:
- Around line 675-686: Update the null-order selection in the order_by handling
of the rolling expression so reverse=True flips null precedence along with sort
direction. Adjust the condition used for nulls.append, preserving the existing
ob_desc and ob_nulls_last behavior when reverse is false.
- Around line 1080-1089: Update the cum_reverse extraction in the
cumulative-expression grouping logic to unwrap fill_null_with_strategy
expressions and read the nested cum_sum reverse flag rather than the fill
strategy at ne.value.options[0]. Preserve direct cum_sum handling, ensure
forward and reverse filled cumulative expressions route to their respective
scans, and add regression coverage for both cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 78be152c-63ca-44d8-8141-c794b6324998
📒 Files selected for processing (1)
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py
|
/merge |
4 participants
Description
xref #23151
https://docs.pola.rs/api/python/stable/reference/expressions/api/polars.Expr.reverse.html
Checklist