feat(cudf): Add CudfGroupId operator for GPU-accelerated GROUPING SETS#17238
Open
shrshi wants to merge 5 commits intofacebookincubator:mainfrom
Open
feat(cudf): Add CudfGroupId operator for GPU-accelerated GROUPING SETS#17238shrshi wants to merge 5 commits intofacebookincubator:mainfrom
shrshi wants to merge 5 commits intofacebookincubator:mainfrom
Conversation
Implement CudfGroupId operator to replace the CPU GroupId operator on GPU for SQL GROUPING SETS, CUBE, and ROLLUP operations. - Add CudfGroupId class inheriting from CudfOperatorBase - Cycle through grouping sets one at a time (matching CPU behavior) - Create all-null columns for keys not in current grouping set - Create constant group_id column for each grouping set - Optimize column ownership with usage counting (move vs copy) - Register GroupIdAdapter in OperatorAdapters - Add comprehensive tests matching core Velox test patterns
shrshi
requested review from
bdice,
devavret,
karthikeyann and
mhaseeb123
as code owners
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
Bot
added
the
CLA Signed
Build Impact AnalysisFull build recommended. Files outside the dependency graph changed:
These directories are not fully covered by the dependency graph. A full build is the safest option. Slow path • Graph generated from PR branch |
Add test that exercises the column copy vs move logic when the same input column appears in multiple grouping sets.
Only move columns on the last grouping set since inputColumns_ is reused across multiple getOutput() calls. This is simpler and avoids potential issues when the same input column maps to multiple output positions. Add test for same input column mapped to multiple outputs in a grouping set.
Fix a bug where the same input column mapped to multiple output positions (e.g., aliased keys) would be moved twice on the last grouping set, leaving a null unique_ptr and crashing on table construction. Track per-call reference counts so columns are moved only on their final use. Also precompute cudf data types for grouping key columns in the constructor instead of recomputing them on every doGetOutput() call.
simoneves
suggested changes
May 5, 2026
Collaborator
simoneves
left a comment
simoneves
left a comment
There was a problem hiding this comment.
Not familiar with this code, so style review only (sorry!)
| size_t groupingSetIndex_{0}; | ||
|
|
||
| /// Total number of grouping sets. | ||
| size_t numGroupingSets_; |
Collaborator
There was a problem hiding this comment.
Nit: init to zero (at least for consistency with the others, even though it's set in the constructor)
| size_t numGroupingSets_; | ||
|
|
||
| /// Number of grouping key columns in output. | ||
| size_t numGroupingKeys_; |
| auto tempMr = get_temp_mr(); | ||
| auto numRows = inputSize_; | ||
|
|
||
| const auto& mapping = groupingKeyMappings_[groupingSetIndex_]; |
Collaborator
There was a problem hiding this comment.
Nit: CHECK on index before use.
| auto inputIdx = aggregationInputs_[i]; | ||
| auto outputIdx = numGroupingKeys_ + i; | ||
| if (isLastGroupingSet && --remainingUses[inputIdx] == 0) { | ||
| outputColumns[outputIdx] = std::move(inputColumns_[inputIdx]); |
Collaborator
There was a problem hiding this comment.
Nit: CHECK on indices before use (4x)
| 0, | ||
| "GroupIdNode didn't map grouping key {} to input channel", | ||
| groupingKey); | ||
| auto inputChannel = outputToInputGroupingKeyMapping.at(outputChannel); |
Collaborator
There was a problem hiding this comment.
Nit: CHECK result of at()
| "GroupIdNode didn't map grouping key {} to input channel", | ||
| groupingKey); | ||
| auto inputChannel = outputToInputGroupingKeyMapping.at(outputChannel); | ||
| mappings[outputChannel] = inputChannel; |
Collaborator
There was a problem hiding this comment.
Nit: CHECK index before use.
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
3 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.
Summary
CudfGroupIdoperator to replace the CPUGroupIdoperator on GPU for SQLGROUPING SETS,CUBE, andROLLUPoperationsImplementation Details
The
CudfGroupIdoperator:group_idcolumn (BIGINT) for each grouping setTesting
Added
GroupIdTest.cppwith tests matching core Velox patterns inAggregationTest.cpp: