[VL] Materialize CudfVector to CPU columns before host code reads them#12416
Open
ReemaAlzaid wants to merge 2 commits into
Open
[VL] Materialize CudfVector to CPU columns before host code reads them#12416ReemaAlzaid wants to merge 2 commits into
ReemaAlzaid wants to merge 2 commits into
Conversation
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.
What changes are proposed in this pull request?
With the Velox cuDF (GPU) backend, GPU results are wrapped in
CudfVectoraRowVectorsubclass that keeps its columns in a
cudf::tableon the device and is constructed withno CPU children (
childrenSize_ == 0).VeloxColumnarBatch::fromhands these batches tohost-side code as-is, without ever copying the columns down from the GPU. Any CPU code that
then reads a child column crashes:
This was observed in two host side paths:
RowVectorStream::next/CudfVectorStreamBase::next), e.g. when Sparksamples a global
ORDER BYviaRangePartitionerand evaluates a projection over thesampled batch;
JniHashTable→HashTableBuilder::addInput), which readsjoin-key children directly.
This PR adds
gluten::materializeVeloxRowVector()(cpp/velox/utils/CudfVectorUtils.h): for aCudfVectorit copies the real GPU columns into CPU-resident Velox children viacudf_velox::with_arrow::toVeloxColumn()(the exact inverse of how theCudfVectorwas built —nothing dropped or fabricated); for any other vector, and for non-GPU builds
(
#ifndef GLUTEN_ENABLE_GPU), it returns the input unchanged. It is called immediately beforeevery host-side child access:
RowVectorStream,CudfVectorStream, theJniHashTablebuildloop,
VeloxColumnarBatch::{ensureFlattened,compose,select,toUnsafeRow}, the columnar-to-rowprune in
VeloxJniWrapper, andVeloxBatchResizer.The change is inert for the CPU backend (compile-guarded + a runtime
CudfVectortype check →no-op passthrough), so it does not affect non-GPU execution.
This is a correctness fix at the GPU→CPU boundary. Keeping these operators fully on the GPU (so
no materialization is needed) is separate follow-up work; likewise the broadcast-join CUDA
build/probe ordering race (Velox #17758), the cuDF expression gaps, and the
CudfTopNnull-input crash are out of scope here.
How was this patch tested?
cpp/velox/tests/VeloxGpuShuffleWriterTest.cccovering theCudfVector→ host
RowVectormaterialization path.--decimal-as-doubleviagluten-it queries-compare(GPU output diffedagainst vanilla Spark, row and value),
spark.gluten.sql.columnar.cudf=true, on2× NVIDIA L40S / CUDA 12.9.
Before this patch, the GPU path crashed with
childAt (N vs 0)on every query whose plancarried a
CudfVectorinto a host-side operator (all global-ORDER BYand broadcast-joinqueries). After this patch the crashes are gone and 12 of 22 queries are value-correct on the
GPU path (output identical to vanilla Spark), with 2–4× speedups:
Fixed / value-correct (12):
q1, q4, q5, q6, q7, q9, q11, q13, q14, q16, q21, q22(e.g. q1 = 4 rows, q9 = 175, q16 = 27840, q21 = 100 — all matching vanilla).
Still failing — separate, out-of-scope follow-ups (10):
q2, q18, q20q8, q10, q12q17, q19q3, q15CudfTopN::doAddInputnull inputThese are independent of the
CudfVectormaterialization boundary and are tracked separately.CPU Gluten (
cudf=false) remains correct for all 22 and is unaffected by this change.