fix(cudf): Skip input conversion for GPU source nodes - #58
Merged
Conversation
Co-authored-by: Ferdinand Xu <ferdinandx@nvidia.com>
sperlingxx
marked this pull request as ready for review
August 13, 2026 03:42
winningsix
approved these changes
Aug 13, 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
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
Avoid inserting
CudfFromVeloxbefore a GPU source operator whose plan nodehas no input edge. Add focused selection tests for both sides of the boundary:
Fixes #59.
Root cause
Commit
07e146e88bmade the first operator in a fragment count as a non-GPUpredecessor. That is required when an MPP/UCX fragment receives an external
Velox
RowVector, but it also matched GPU source leaves. The compiler thenasked
gpuInputBoundaryType()to classify a source that did not exist andraised:
This change additionally requires an input plan edge before inserting the
boundary. A first operator with a real input edge keeps the behavior introduced
by
07e146e88b.Upstream and overlap check
The failing integration build used
devcommit0af87d87e6. Currentdevcommit
de935de174has the sameToCudf.cppbytes, including the faultyfirst-operator condition, so the issue remains on current
dev.PR #39 is the only open PR found with this guard, but it is a large,
conflicting TopN/Grace/UCX change. No focused open
ToCudfPR or issue wasfound. Issue #27 is a general CI-coverage roadmap and does not implement this
fix.
This PR extracts the guard from #39 and adds dedicated boundary tests so it can
be reviewed and landed independently. If either PR lands first, the duplicate
hunk should be removed from the other during rebase. The original author is
credited in the commit trailer.
Validation
Paired Spark qualification used Gluten
0c5a3dd271and this Velox commit0ac015ae15, producing runtime imagesha256:a1793e9702106fa425fb5832d2cd74c9f6565754db24565a302131f048c24de3.velox_cudf_tocudf_selection_test: 24/25 passed. The one failure,complexGroupingKeyExpressionsFallsBack, is an independently reproducedcurrent-
devaggregation-selection failure; this boundary guard is not onthat plan's changed path.
FluxStrategyPlanSuitecases: 22/22 passed.checks; 22 passed, 0 failed, 0 errors.
ucx_exchange_test: 204 registered, 78 run, 126 expected skips,0 failures.
singleTaskMode=false, 5 fragments and 4 exchanges (2 HASH, 1 BROADCAST,1 SINGLE). Remote UCX handshakes reported
isIntraNodeTransfer=0and no GPUinput-boundary exception.
clang-format --dry-run --Werrorandgit diff --check: passed.