Refactor groupby grouping and Python ordering by PointKernel · Pull Request #24562 · NVIDIA/cudf

PointKernel · 2026-10-12T20:01:23Z

Description

This PR separates HashCSR key grouping from aggregation and reuses group ordering and row mappings in cuDF Python and Polars. It preserves pandas-compatible group order and correctly restores row positions after apply and window operations.

Split out from #24251.

Checklist

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

…ouping-preparation

copy-pr-bot · 2026-10-12T20:01:26Z

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.

PointKernel · 2026-10-12T20:09:41Z

/ok to test

coderabbitai · 2026-10-12T23:52:16Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Last reviewed commit: 75027e1
Latest commit, not reviewed: c2011d1

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: NVIDIA/cudf/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d0d6168f-f075-4d91-89fa-08b97b1bcc62

📥 Commits

Reviewing files that changed from the base of the PR and between 09e2c41 and 75027e1.

📒 Files selected for processing: 3

cpp/src/groupby/hash/group_keys.cu
cpp/src/groupby/hash/group_keys.hpp
python/cudf/cudf/core/groupby/groupby.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.

📝 Summary

Summary by CodeRabbit

Bug Fixes
- GroupBy operations with sort=False now consistently preserve first-appearance group order across iteration, aggregation, apply, row selection, and scan results.
- Pandas-compatible grouping now applies the expected ordering to reduction results, including when sorting is disabled.
- Grouped rolling operations now retain input row order when reordering results.
- Hash-based grouping now handles aggregation requests and includes or excludes null keys according to the selected policy.

The PR separates C++ hash key grouping from aggregation execution. It centralizes cuDF Python group ordering and changes Polars rolling expressions to use row IDs returned by the grouper when restoring input order.

Changes: C++ Hash Groupby

Layer / File(s)	Summary
Key grouping contract and implementation `cpp/src/groupby/hash/group_keys.hpp`, `cpp/src/groupby/hash/group_keys.cu`	Adds a grouped-key result and a table-based grouping function. The function handles null-key policy and prepares key comparison and hashing.
Aggregation execution and entry-point wiring `cpp/src/groupby/hash/compute_groupby.hpp`, `cpp/src/groupby/hash/compute_groupby.cpp`, `cpp/src/groupby/hash/groupby.cu`, `cpp/CMakeLists.txt`	Moves aggregation extraction, execution, caching, and compound-aggregation finalization into `compute_groupby`. The groupby entry point calls it directly, and CMake compiles its `.cpp` source.

Group Ordering

Layer / File(s)	Summary
Group order selection and caching `python/cudf/cudf/core/groupby/groupby.py`	Adds ordering controls and caches for group offsets and row permutations. Groups can follow key order or first input-row position.
Ordered groupby operations and validation `python/cudf/cudf/core/groupby/groupby.py`, `python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py`, `python/cudf/cudf/tests/groupby/*`	Routes aggregation, iteration, head and tail, apply, and scan ordering through the ordering controls. Updates groupby tests and removes two expected-failure entries.
Polars rolling row-order restoration `python/cudf_polars/cudf_polars/dsl/expressions/rolling.py`	Uses row IDs returned by the grouper when restoring input order without an explicit order index. Updates the rolling, rank, and fill-null call sites.

Priority: ⬇️ Low

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

Change: Refactor

Merge Risk: ⚪ Minimal · up to 75027

This change restructures how groupby builds key groups and how group order is preserved in Python and Polars. It is intended to keep pandas-compatible ordering. The review found no concrete defect in the supplied changes, so the change looks ready to merge after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 46.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 10 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main changes: refactoring groupby grouping and Python ordering.
Description check		The description directly explains the separation of HashCSR grouping from aggregation and the reuse of ordering and row mappings in cuDF Python and Polars.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

wence-

Approving python changes.

Co-authored-by: Yunsong Wang <12716979+PointKernel@users.noreply.github.com>

mhaseeb123

One nit

mhaseeb123 · 2026-10-15T01:42:05Z

+            else []
        )
+        positions = value_columns.pop() if need_positions else None
+        if not order_groups or len(offsets) <= 2:


nit: get_groups already returns sorted groups with sort=True, so we can return early here. Can also drop the sort=True branch below.

Suggested change

if not order_groups or len(offsets) <= 2:

if not order_groups or self._sort or len(offsets) <= 2:

mhaseeb123 · 2026-10-15T01:42:05Z

-  auto const temporary_resources = mr.as_temporary();
-
-  [[maybe_unused]] auto [row_bitmask_data, row_bitmask] =
+  auto const num_rows = keys.num_rows();


nit: Since need_grouped_rows requires group offsets

Suggested change

auto const num_rows = keys.num_rows();

CUDF_EXPECTS(need_group_offsets || !need_grouped_rows,

"Grouped rows require group offsets",

std::invalid_argument);

auto const num_rows = keys.num_rows();

PointKernel added 4 commits October 12, 2026 12:44

Separate HashCSR key construction from aggregation orchestration

2452235

Preserve Python group ordering independently of the backend

0fa5699

Restore Polars window rows using the grouper permutation

2abae4c

Merge remote-tracking branch 'upstream/main' into refactor/groupby-gr…

a88d6c7

…ouping-preparation

github-actions Bot assigned PointKernel Oct 12, 2026

github-actions Bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. CMake CMake build issue cudf.pandas Issues specific to cudf.pandas cudf-polars Issues specific to cudf-polars labels Oct 12, 2026

github-project-automation Bot added this to cuDF Python Oct 12, 2026

github-project-automation Bot moved this to Todo in cuDF Python Oct 12, 2026

PointKernel added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Oct 12, 2026

PointKernel marked this pull request as ready for review October 12, 2026 23:46

PointKernel requested review from a team as code owners October 12, 2026 23:46

PointKernel requested review from mhaseeb123, rjzamora and shrshi October 12, 2026 23:46

wence- approved these changes Oct 13, 2026

View reviewed changes

PointKernel commented Oct 13, 2026

View reviewed changes

Comment thread cpp/src/groupby/hash/group_keys.hpp Outdated

Comment thread cpp/src/groupby/hash/compute_groupby.cpp Outdated

PointKernel and others added 3 commits October 13, 2026 09:39

Apply batched suggestions from code review

40b1bda

Co-authored-by: Yunsong Wang <12716979+PointKernel@users.noreply.github.com>

Merge branch 'main' into refactor/groupby-grouping-preparation

d5b1360

Merge main into refactor/groupby-grouping-preparation

9824737

Merge branch 'main' into refactor/groupby-grouping-preparation

09e2c41

github-actions Bot assigned vyasr Oct 15, 2026

mhaseeb123 approved these changes Oct 15, 2026

View reviewed changes

PointKernel added 4 commits October 15, 2026 16:48

Merge upstream/main into refactor/groupby-grouping-preparation

bfc2784

Address grouping order and precondition review comments

87b04dc

Merge latest upstream/main into refactor/groupby-grouping-preparation

75027e1

Merge upstream/main into refactor/groupby-grouping-preparation

c2011d1

Navigation Menu

Repository navigation

Refactor groupby grouping and Python ordering - #24562

Refactor groupby grouping and Python ordering#24562
PointKernel wants to merge 12 commits into
NVIDIA:mainfrom
PointKernel:refactor/groupby-grouping-preparation

PointKernel commented Oct 12, 2026 •

edited

Loading

Uh oh!

copy-pr-bot Bot commented Oct 12, 2026

Uh oh!

PointKernel commented Oct 12, 2026

Uh oh!

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading

Reviews paused

Summary by CodeRabbit

Uh oh!

wence- left a comment

Uh oh!

Uh oh!

Uh oh!

mhaseeb123 left a comment

Uh oh!

mhaseeb123 Oct 15, 2026

Uh oh!

mhaseeb123 Oct 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if not order_groups or len(offsets) <= 2:
	if not order_groups or self._sort or len(offsets) <= 2:

-  auto const num_rows = keys.num_rows();
+  CUDF_EXPECTS(need_group_offsets || !need_grouped_rows,
+               "Grouped rows require group offsets",
+               std::invalid_argument);
+  auto const num_rows = keys.num_rows();

Conversation

PointKernel commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

copy-pr-bot Bot commented Oct 12, 2026

Uh oh!

PointKernel commented Oct 12, 2026

Uh oh!

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

Uh oh!

Reviews paused

Summary by CodeRabbit

Uh oh!

wence- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhaseeb123 left a comment

Choose a reason for hiding this comment

Uh oh!

mhaseeb123 Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

mhaseeb123 Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PointKernel commented Oct 12, 2026 •

edited

Loading

coderabbitai Bot commented Oct 12, 2026 •

edited

Loading