Fix cum_sum window functions with null filling in cudf-polars by Matt711 · Pull Request #23206 · NVIDIA/cudf

Matt711 · 2026-07-09T19:27:46Z

Description

Makes expressions like this work correctly

pl.col("a")
    .cum_sum()
    .fill_null(...)
    .over(...)

Closes #23189

This PR also closes #23205 by falling back to a single partition when executing cum_sum window functions without an order_by set in over with multiple ranks.

Needed for #22868 because of the change to Q51.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

copy-pr-bot · 2026-07-09T19:27:50Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

Matt711 · 2026-07-09T19:28:06Z

/ok to test d72e3cb

coderabbitai · 2026-07-10T15:14:02Z

📝 Walkthrough

Summary by CodeRabbit

New Features
- Improved support for cumulative sums with forward or backward null filling in windowed operations.
- Added correct handling for grouped cumulative sums without an explicit ordering column.
Bug Fixes
- Improved multi-partition processing for cumulative sum windows.
- Added clear errors for unsupported null-filling combinations with window functions.
Tests
- Added coverage for supported cumulative sum and null-filling strategies.
- Expanded validation of window-function behavior and streaming execution.

Walkthrough

Changes

Cumulative window support

Layer / File(s)	Summary
Per-partition cumulative fill execution `python/cudf_polars/cudf_polars/dsl/expressions/rolling.py`	Cumulative sums separate wrapped fill strategies, enforce sorted groups when needed, and apply forward or backward null replacement after grouped aggregation.
Window translation validation `python/cudf_polars/cudf_polars/dsl/translate.py`, `python/cudf_polars/tests/streaming/test_rolling.py`, `python/cudf_polars/cudf_polars/testing/inject_gpu_engine.py`	Window translation handles wrapped cumulative sums, rejects filled rank expressions, and adds corresponding tests and expected-failure updates.
Distributed cumulative-sum fallback `python/cudf_polars/cudf_polars/streaming/utils.py`, `python/cudf_polars/cudf_polars/streaming/select.py`	Multi-rank lowering detects cumulative sums without `order_by` and falls back to the existing single-partition-style path.

Estimated code review effort: 4 (Complex) | ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: cum_sum window behavior with null filling in cudf-polars.
Description check	✅ Passed	The description is directly related to the changes and correctly describes the cum_sum window fixes and fallback behavior.
Linked Issues check	✅ Passed	The changes address both linked bugs by fixing cum_sum null-filling per partition and adding a multi-rank no-order_by fallback.
Out of Scope Changes check	✅ Passed	No clear out-of-scope changes are present; the test expectation updates and minor header tweak support the same fixes.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

mroeschke · 2026-07-10T16:47:08Z

+                and isinstance(v.children[0], UnaryFunction)
+            ):
+                v = v.children[0]
+            if isinstance(v, UnaryFunction) and v.name == "cum_sum":


question: Does this bug apply to any other of the cumulative functions besides cum_sum?

Yes but cum_sum is the only one we support for window functions

Gotcha, I would learn towards checking v.name.startswith("cum_") for a little more future-proofing but not a blocker.

mroeschke · 2026-07-10T16:59:20Z

+                and isinstance(v.children[0], expr.UnaryFunction)
+                and v.children[0].name == "rank"


question (not sure if possible): But can v.children[0] be an expression who's child is also a expr.UnaryFunction? i.e do we need to traverse potentially nested children to find a rank expr.UnaryFunction?

Good question, we're pretty limited on the window functions we allow. But I'll double-check

Thanks, this is a problem and I switched to falling back on the nested unsupported cases in 9f4a5c6

coderabbitai

🧹 Nitpick comments (1)

python/cudf_polars/tests/streaming/test_rolling.py (1)
97-108: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider adding all-null/single-element partition cases.

The new test only covers partitions with partial nulls; per-partition edge cases like an all-null partition or a single-row partition for cum_sum().fill_null() aren't exercised here.

As per path instructions, "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/streaming/test_rolling.py` around lines 97 - 108,
The test_over_cum_sum_fill_null_per_partition parametrized case only covers
partially null partitions; extend its test data or add focused cases to cover an
all-null partition and a single-row partition while retaining both forward and
backward strategies. Keep the cases self-contained and validate them through
assert_gpu_result_equal.
Source: Path instructions

🤖 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.

Nitpick comments:
In `@python/cudf_polars/tests/streaming/test_rolling.py`:
- Around line 97-108: The test_over_cum_sum_fill_null_per_partition parametrized
case only covers partially null partitions; extend its test data or add focused
cases to cover an all-null partition and a single-row partition while retaining
both forward and backward strategies. Keep the cases self-contained and validate
them through assert_gpu_result_equal.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8a83cc01-6ab9-4a11-99d8-21da428ceea0

📥 Commits

Reviewing files that changed from the base of the PR and between d72e3cb and 2081139.

📒 Files selected for processing (6)

python/cudf_polars/cudf_polars/dsl/expressions/rolling.py
python/cudf_polars/cudf_polars/dsl/translate.py
python/cudf_polars/cudf_polars/streaming/select.py
python/cudf_polars/cudf_polars/streaming/utils.py
python/cudf_polars/cudf_polars/testing/inject_gpu_engine.py
python/cudf_polars/tests/streaming/test_rolling.py

💤 Files with no reviewable changes (1)

python/cudf_polars/cudf_polars/testing/inject_gpu_engine.py

🚧 Files skipped from review as they are similar to previous changes (4)

python/cudf_polars/cudf_polars/streaming/utils.py
python/cudf_polars/cudf_polars/streaming/select.py
python/cudf_polars/cudf_polars/dsl/translate.py
python/cudf_polars/cudf_polars/dsl/expressions/rolling.py

mroeschke · 2026-07-10T18:43:00Z

+    windowed = [
+        node
+        for node in traversal([value])
+        if isinstance(node, expr.UnaryFunction) and node.name in {"rank", "cum_sum"}


Can you remind me why we need to check rank but not in the last child.name check below?

Also a small docstring just describing what we're disallowing would be helpful

It's because the only unary functions we actually care about / support (other than fill null itself) are rank and cum_sum. This first is used to filter for both. And the second check only allows cum_sum because we only support fill_null(cum_sum(...)) not fill_null(rank(...)).

Added docs in 215bc79

But you're right this isn't easy to reason about since you have to know which window functions are supported. In a follow-up, I'll do some refactoring to make this easier to understand.

Matt711 · 2026-07-20T20:20:36Z

/merge

Fix cumulative window functions with null filling in cudf-polars

d569b95

Matt711 added bug Something isn't working non-breaking Non-breaking change labels Jul 9, 2026

github-actions Bot assigned Matt711 Jul 9, 2026

github-actions Bot added Python Affects Python cuDF API. cudf-polars Issues specific to cudf-polars labels Jul 9, 2026

github-project-automation Bot added this to cuDF Python Jul 9, 2026

GPUtester moved this to In Progress in cuDF Python Jul 9, 2026

Matt711 marked this pull request as ready for review July 10, 2026 15:08

Matt711 requested a review from a team as a code owner July 10, 2026 15:08

Matt711 requested a review from mroeschke July 10, 2026 15:08

mroeschke reviewed Jul 10, 2026

View reviewed changes

address review unsuported complex window functions

9f4a5c6

Matt711 force-pushed the bug/polars/cum-sum-null-filling branch from d72e3cb to 9f4a5c6 Compare July 10, 2026 18:07

Merge branch 'main' into bug/polars/cum-sum-null-filling

2081139

coderabbitai Bot reviewed Jul 10, 2026

View reviewed changes

mroeschke approved these changes Jul 10, 2026

View reviewed changes

Matt711 and others added 6 commits July 11, 2026 12:06

Merge branch 'main' into bug/polars/cum-sum-null-filling

8be3c14

Merge branch 'main' into bug/polars/cum-sum-null-filling

b50ec57

Merge branch 'main' into bug/polars/cum-sum-null-filling

fc357e0

Merge branch 'main' into bug/polars/cum-sum-null-filling

83a0102

Merge branch 'main' into bug/polars/cum-sum-null-filling

bfaed26

add docs

215bc79

rapids-bot Bot merged commit 69abe4c into NVIDIA:main Jul 20, 2026
107 of 108 checks passed

github-project-automation Bot moved this from In Progress to Done in cuDF Python Jul 20, 2026

Matt711 deleted the bug/polars/cum-sum-null-filling branch July 20, 2026 23:00

This was referenced Jul 21, 2026

Support pl.Expr.reverse, reverse=True for Polars cummulative expressions #23164

Merged

Support shift over grouped windows in cudf-polars #23451

Merged

coderabbitai Bot mentioned this pull request Jul 28, 2026

Preserve input order for grouped window evaluation #23466

Merged

3 tasks

josephine-wolf-oberholtzer added this to cuDF Python Aug 12, 2026

josephine-wolf-oberholtzer moved this to Done in cuDF Python Aug 12, 2026

		and isinstance(v.children[0], expr.UnaryFunction)
		and v.children[0].name == "rank"

Uh oh!

Conversation

Matt711 commented Jul 9, 2026

Description

Checklist

Uh oh!

copy-pr-bot Bot commented Jul 9, 2026

Uh oh!

Matt711 commented Jul 9, 2026

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mroeschke Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matt711 commented Jul 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

mroeschke Jul 10, 2026 •

edited

Loading