Skip to content

[bug] cuDF aggregation selection accepts unsupported grouping keys and stale max(boolean) #107

Description

@sperlingxx

Problem

Part of umbrella #100 (velox_cudf_aggregation_selection_test). Same grouping-key helper also fails ToCudfSelectionTest.complexGroupingKeyExpressionsFallsBack.

On current dev:

  • CudfAggregationSelectionTest.complexGroupbyClauseExpressions: canBeEvaluatedByCudf returns true, expected false. Plan is Project(to_big_endian_64(c0) AS endian_c0) then GROUP BY endian_c0. to_big_endian_64 is not a cuDF expression.
  • CudfAggregationSelectionTest.invalidTypeCombinationsRejected: canAggregationBeEvaluatedByCudf(max(boolean)) returns true, expected false. avg(varchar) and sum(varchar) already reject. Only max(boolean) is stale.
  • ToCudfSelectionTest.complexGroupingKeyExpressionsFallsBack: wasCudfAggregationUsed is true, expected false. Same grouping-key expansion as the first case.

What we keep vs reject from 13b1de5e0

13b1de5e0 (Resolve projected aggregate inputs for cuDF, Ferdinand Xu, 2026-06-27) did two things in one commit.

Keep (runtime projected aggregate inputs)

normalizeProjectInputReferences in buildAggregationInputChannels.

Spark-style Project then Agg needs this: remap unresolved identity aliases onto Project output names; if a FieldAccess already exists on the Project output, bind it as a channel; evaluate a real expression as a precomputed column. Do not substitute the originating Project expression at runtime (that would re-run to_big_endian_64 / plus inside CudfGroupby).

Example that must still use CudfGroupby:

Project(plus(c1, c2) AS y) → sum(y)

Selection still sees plus (GPU-ok). Runtime binds y as a column and does not re-evaluate plus.

Reject (unifying eligibility with that remapper)

13b1de5e0 also pointed expandFieldReference at normalizeProjectInputReferences. Runtime had already stopped calling expandFieldReference. Selection in CudfGroupby / CudfReduce / canGroupingKeysBeEvaluatedByCudf still does. The wrapper made selection look at Project output column names, so GROUP BY endian_c0 looked like a normal column even though it came from to_big_endian_64.

That was not a documented eligibility redesign (no test updates, message is only about projected inputs). It broke the Meta contract from facebookincubator#15529: eligibility must expand FieldAccess through Project and reject GPU aggregation when the originating expression cannot run on GPU.

Example that must not use CudfGroupby:

Project(to_big_endian_64(c0) AS k) → GROUP BY k

Other case: max(boolean)

635816cba registered min/max on boolean. GroupbyMaxAggregator is real cuDF MAX. The reject-list test was never updated. Move boolean min/max to the supported-signature list. Keep rejecting avg(varchar), sum(varchar), and max(array).

Acceptance criteria

  • Restore Project-expression expansion in expandFieldReference (selection only).
  • Leave normalizeProjectInputReferences on the runtime channel path.
  • complexGroupbyClauseExpressions and complexGroupingKeyExpressionsFallsBack reject GPU aggregation.
  • max(boolean) is treated as supported; invalidTypeCombinationsRejected still rejects unregistered type combos.

Validation

Reproduced and verified on HPDA dev 28798f623 (CUDA SM 86):

Prepared with assistance from Cursor.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingcudf

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions