fix(cudf): Restore Project expansion for agg eligibility - #108
Open
sperlingxx wants to merge 3 commits into
Open
fix(cudf): Restore Project expansion for agg eligibility#108sperlingxx wants to merge 3 commits into
sperlingxx wants to merge 3 commits into
Conversation
Grouping by a column produced by to_big_endian_64 was accepted for CudfGroupby because eligibility remapped the Project output name instead of inspecting the originating expression. Keep runtime alias remapping for projected aggregate inputs. Restore FieldAccess expansion through Project only for GPU selection. Fixes rapidsai#107 Part of rapidsai#100
This was referenced Sep 1, 2026
Drop the header comment and selection-test edits. Only expandFieldReference is restored to Project-expression expansion.
sperlingxx
changed the title
sperlingxx
changed the title
sperlingxx
changed the title
sperlingxx
changed the title
Collaborator
Author
|
build |
Boolean min/max are registered GPU signatures. The reject test still expected max(boolean) to be ineligible. Cover min/max boolean in comprehensiveTypeSupportValidation and leave avg/sum varchar as rejects.
winningsix
approved these changes
Sep 3, 2026
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
2 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
expandFieldReferenceso GPU aggregation eligibility seesto_big_endian_64(c0), not just the projected field nameendian_c0.normalizeProjectInputReferenceson the runtime channel path from13b1de5e0(Resolve projected aggregate inputs for cuDF).min/max(boolean)onto the accept list. Those signatures were registered in635816cba; the reject test still expectedmax(boolean)to be ineligible. Leaveavg(varchar)andsum(varchar)as rejects.What we keep vs reject from
13b1de5e0Keep — runtime projected aggregate inputs. Spark-style
ProjectthenAggstill remaps unresolved identity aliases onto Project output names. A FieldAccess that already exists on the Project output is a column channel. A real expression is a precomputed column. Runtime does not substitute the originating Project expression (that would re-runplus/to_big_endian_64inside CudfGroupby).Selection still sees
plus(GPU-ok). Runtime bindsyas a column.Reject — unifying eligibility with that remapper.
13b1de5e0pointedexpandFieldReferenceat the alias remapper even though runtime had already stopped calling it. Selection in CudfGroupby / CudfReduce still calls it. Grouping byendian_c0then looked like a normal column, so CudfGroupby was selected for a GPU-unsupported origin expression.Must stay on CPU HashAggregation. That is the Meta contract from facebookincubator#15529, not an undo of projected-input execution.
Test plan
dev28798f623(CUDA SM 86): both aggregation-selection cases fail as in [umbrella] velox_cudf_test: 12 failed CTest suites #100.complexGroupbyClauseExpressionspasses andinvalidTypeCombinationsRejectedno longer asserts against registered boolean min/max.velox_cudf_aggregation_selection_test.velox_cudf_tocudf_selection_test(includescomplexGroupingKeyExpressionsFallsBack).Fixes #107
Part of #100
Prepared with assistance from Cursor.