Skip to content

fix(cudf): Restore Spark workload coverage - #54

Merged
thirtiseven merged 7 commits into
HighPerfDataAccelerator:devfrom
thirtiseven:agent/restore-spark-cudf-expression-coverage
Aug 12, 2026
Merged

fix(cudf): Restore Spark workload coverage#54
thirtiseven merged 7 commits into
HighPerfDataAccelerator:devfrom
thirtiseven:agent/restore-spark-cudf-expression-coverage

Conversation

@thirtiseven

@thirtiseven thirtiseven commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • restore cuDF evaluation for Spark regexp_extract, numeric-to-VARCHAR
    casts, isnull, dynamic array, and multi-branch switch expressions
  • restore CudfUnnest to the cuDF exec build and operator adapter registry
  • allow partial-identity aggregation without a streaming distinct-key capacity
  • transfer partial-identity input column ownership instead of deep-copying every grouping key and aggregate input

Motivation

The TypedExpr expression-subsystem merge dropped coverage that existed in the previous exec::Expr implementation. Strict functional canaries exposed regressions across scalar expression evaluation, multi-branch switch, and unnest operator selection.

Large-scale aggregation validation additionally exposed two independent partial-identity issues: enabling partial identity was incorrectly coupled to a non-zero streaming capacity, and partial identity deep-copied its complete input. The ownership transfer follows the existing CudfFilterProject pattern and copies only when a source column has multiple consumers.

Test plan

Native GPU tests:

  • AggregationTest.partialIdentityUsesQueryScopedStreamingCapacity
  • AggregationTest.partialIdentityDoesNotRequireStreamingCapacity
  • CudfExpressionSelectionTest.multiBranchSwitch

Production build:

  • velox_cudf_exec compiled successfully
  • Spark Gluten 4.0 / Java 17 / Scala 2.13 bundle packaged successfully

100% local MPP validation (4 executors / 4 x 32 GiB GPUs, CPU fallback disabled):

  • unnest-heavy query: SUCCESS, 95.743 s with the normal async allocator
  • aggregation-heavy query: SUCCESS, 465.777 s with managed_async

The aggregation-heavy query still outgrows a 32 GiB GPU with async. Device diagnostics show about 33.25 GB of live RMM allocations while the FINAL group-by accumulates state, so this is not retained allocator cache. managed_async validates the fixed operator paths and correctness at full scale, but it is not a performance configuration. A separate follow-up should add FINAL group-by spill or test a topology with more physical MPP partitions.

@github-actions github-actions Bot added the cudf label Aug 11, 2026
@thirtiseven thirtiseven changed the title fix(cudf): restore Spark expression coverage fix(cudf): Restore Spark workload coverage Aug 11, 2026
@thirtiseven
thirtiseven requested a balanced review from Copilot August 12, 2026 01:36
@thirtiseven
thirtiseven marked this pull request as ready for review August 12, 2026 01:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores cuDF coverage for Spark expressions, unnest execution, and partial-identity aggregation.

Changes:

  • Adds Spark expression support and parity tests.
  • Re-registers CudfUnnest.
  • Optimizes partial-identity aggregation ownership and capacity handling.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
FilterProjectTest.cpp Adds expression parity tests.
ExpressionEvaluatorSelectionTest.cpp Tests GPU expression selection.
AggregationTest.cpp Tests zero-capacity partial identity.
AdapterOperatorTest.cpp Verifies cuDF unnest selection.
ExpressionEvaluator.cpp Implements restored Spark expressions.
AstExpressionUtils.h Adds the isnull alias.
OperatorAdapters.cpp Registers the unnest adapter.
CudfGroupby.h Extends partial-identity ownership API.
CudfGroupby.cpp Transfers input ownership during partial identity.
CMakeLists.txt Restores CudfUnnest.cpp to the build.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread velox/experimental/cudf/exec/CudfGroupby.cpp Outdated
Comment thread velox/experimental/cudf/expression/ExpressionEvaluator.cpp Outdated
Comment thread velox/experimental/cudf/expression/ExpressionEvaluator.cpp
@thirtiseven
thirtiseven requested a balanced review from Copilot August 12, 2026 02:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@winningsix

Copy link
Copy Markdown
Collaborator

there's a failing CI job. Please help address it before merging.

@thirtiseven

Copy link
Copy Markdown
Collaborator Author

there's a failing CI job. Please help address it before merging.

It is failing for a while and some changes is not related to this pr. Let me try to fix it in another PR.

@thirtiseven
thirtiseven merged commit de935de into HighPerfDataAccelerator:dev Aug 12, 2026
2 of 3 checks passed
sperlingxx added a commit that referenced this pull request Sep 1, 2026
- Rewrite `CudfFilterProjectTest.multiBranchSwitchWithRegexpExtract` WHEN predicates from SQL `=` to Spark `equalto(...)`.
- DuckParser maps `=` to Presto `eq`, which the Spark fixture does not register, so the test died in `resolveScalarFunctionType` before SWITCH ran.
- The invalid Spark CASE WHEN text was introduced in #54 (`382e95fdd`). This is a test-dialect fix, not a production SWITCH/`regexp_extract` change.

Fixes #109
Part of #100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants