Recognize named-aggregation lambdas as scans in GroupBy - #23300
Merged
rapids-bot[bot] merged 7 commits intoJul 21, 2026
Merged
Conversation
galipremsagar
added
bug
|
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:
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesGroupby scan lambda handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
python/cudf/cudf/core/groupby/groupby.py (1)
218-239: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache resolved callable names before computing both predicates.
get_nameexecutes the callable probe, but_is_all_scan_aggregatecalls it separately inall()andany(). Materialize the flattened resolved names once, then derive both predicates from that list; ideally reuse the aggregation resolution already performed bymake_aggregation.🤖 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/cudf/core/groupby/groupby.py` around lines 218 - 239, Update the groupby aggregation-name resolution around get_name and _is_all_scan_aggregate to materialize each flattened callable’s resolved name once, then compute both all-scan and any-scan predicates from that cached list. Reuse the names produced during make_aggregation when available, avoiding repeated callable probes while preserving existing handling for non-callables and true UDFs.
🤖 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/cudf/tests/groupby/test_transform.py`:
- Around line 82-91: Expand test_transform_scan_lambda to parameterize both
cumsum and cumprod across empty, all-null, single-element, and mixed-type
inputs, while retaining expected pandas-versus-cuDF comparisons for each case.
Add the repository-required unit benchmark covering this scan-lambda regression.
---
Nitpick comments:
In `@python/cudf/cudf/core/groupby/groupby.py`:
- Around line 218-239: Update the groupby aggregation-name resolution around
get_name and _is_all_scan_aggregate to materialize each flattened callable’s
resolved name once, then compute both all-scan and any-scan predicates from that
cached list. Reuse the names produced during make_aggregation when available,
avoiding repeated callable probes while preserving existing handling for
non-callables and true UDFs.
🪄 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: 6c499992-a1df-4636-84be-e14d105a6a93
📒 Files selected for processing (3)
python/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/groupby/test_transform.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Comment on lines
+82
to
+91
| def test_transform_scan_lambda(): | ||
| # a named-aggregation lambda resolving to a scan must scan per group, | ||
| # not broadcast the group total | ||
| pdf = pd.DataFrame({"key": [0, 0, 1, 1], "val": [1.0, 2.0, 3.0, 4.0]}) | ||
| gdf = cudf.DataFrame(pdf) | ||
|
|
||
| expect = pdf.groupby("key")["val"].transform(lambda x: x.cumsum()) | ||
| got = gdf.groupby("key")["val"].transform(lambda x: x.cumsum()) | ||
|
|
||
| assert_eq(expect, got) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Expand scan-lambda regression coverage.
This test covers only cumsum on a simple non-null float input. Parameterize the regression for cumsum and cumprod, and add empty, all-null, single-element, and mixed-type cases. Add a unit benchmark for this bug fix as required by the repository guidelines.
🤖 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/cudf/tests/groupby/test_transform.py` around lines 82 - 91,
Expand test_transform_scan_lambda to parameterize both cumsum and cumprod across
empty, all-null, single-element, and mixed-type inputs, while retaining expected
pandas-versus-cuDF comparisons for each case. Add the repository-required unit
benchmark covering this scan-lambda regression.
Source: Coding guidelines
A lambda like ``lambda x: x.cumsum()`` resolves through
make_aggregation's op(Aggregation) protocol, where Aggregation.cumsum
is an alias of sum: the scan/reduction distinction exists only in the
aggregation name. _is_all_scan_aggregate identified scans via
``__name__`` ("<lambda>"), so such transforms silently computed group
totals and broadcast them instead of scanning. Probe callables with a
name-recording stand-in mirroring the op(Aggregation) protocol; true
UDFs raise inside the probe and fall back to ``__name__`` as before.
galipremsagar
force-pushed
the
groupby-lambda-scan-detection
branch
from
July 17, 2026 01:41
42efad0 to
ac77cac
Compare
3 tasks
Contributor
Author
|
/okay to test 76649b0 |
Contributor
Author
|
/okay to test 126a93e |
Contributor
Author
|
/okay to test d1e8eb3 |
vyasr
approved these changes
Jul 20, 2026
vyasr
left a comment
vyasr
left a comment
Contributor
There was a problem hiding this comment.
Wow this is a convoluted fix :( I really wish we didn't have to support agg machinery through callables the way that we do.
galipremsagar
added
the
5 - Ready to Merge
…mbda-scan-detection # Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Contributor
Author
|
/okay to test e514d19 |
…mbda-scan-detection # Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py # python/cudf/cudf/tests/groupby/test_transform.py
Contributor
Author
|
/okay to test 259812d |
Contributor
Author
|
/merge |
davidwendt
pushed a commit
to wjxiz1992/cudf
that referenced
this pull request
Jul 21, 2026
A lambda like `lambda x: x.cumsum()` resolves through `make_aggregation`'s `op(Aggregation)` protocol, where `Aggregation.cumsum` is an alias of `sum`: the scan/reduction distinction exists only in the aggregation name. `_is_all_scan_aggregate` identified scans via `__name__` (`"<lambda>"`), so such transforms silently computed group totals and broadcast them instead of scanning. Probe callables with a name-recording stand-in mirroring the `op(Aggregation)` protocol; true UDFs raise inside the probe and fall back to `__name__` as before. Fixes 2 pandas-tests (`test_cython_transform_series[cumsum/cumprod-<lambda>]`); their xfail entries are removed. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#23300
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
A lambda like
lambda x: x.cumsum()resolves throughmake_aggregation'sop(Aggregation)protocol, whereAggregation.cumsumis an alias ofsum: the scan/reduction distinction exists only in the aggregation name._is_all_scan_aggregateidentified scans via__name__("<lambda>"), so such transforms silently computed group totals and broadcast them instead of scanning. Probe callables with a name-recording stand-in mirroring theop(Aggregation)protocol; true UDFs raise inside the probe and fall back to__name__as before.Fixes 2 pandas-tests (
test_cython_transform_series[cumsum/cumprod-<lambda>]); their xfail entries are removed.Checklist