ReemaAlzaid commented on PR #12471: URL: https://github.com/apache/gluten/pull/12471#issuecomment-5328063033
> @ReemaAlzaid Apologise for missing your previous comment. > > Here's my understanding and some suggestions: > > The first issue `1. Broadcast joins built from an empty stream` can be workaround by setting `spark.gluten.velox.buildHashTableOncePerExecutor.enabled=false` because Velox Cudf doesn't support this feature. This PR disable this optimisation in the code so we don't need to explicitly disable the bhj optimisation for GPU workloads, and I agree with this change. > > The second issue `2. GPU vectors crashing on host reads` has occurred in the previous test but now it cannot be reproduced, while this PR still keep the changes to serve as a temporary remedy in case the CI failure in the future. If so, I do not agree with this change. The major reason is that it still does have the down side to burry some real issues by silencing the conversion, making the real issue hard to trace. In terms of GPU CI, currently we only have the CI job to test the GPU compilation, and there's no test running. If we can have the CI to run GPU testing in the future and once it's broken and needs fix from Velox, we can temporary disable the GPU CI rather than switching on this conversion in the code. @marin-ma no worries, thanks for getting back! I Kept the `VeloxBroadcastBuildSideRDD` change, and the conversion is gone completely `materializeVeloxRowVector`, all call sites, and the strictResidency config. You were right about it hiding things too. Once I removed it, TPC-H on GPU started failing loudly and pointed at two real bugs in the value streams:` RowVectorStream::next()` was rebuilding device batches into RowVectors with no children (the childAt() crash in CudfFromVelox, q17/q19), and CudfValueStream was forwarding host batches straight into GPU-only operators (the cudfInput != nullptr crash in CudfTopN, q18). Both fixed device batches pass through as-is, host batches get uploaded. The matching Velox side fix is up here with tests that reproduce the crash: https://github.com/facebookincubator/velox/pull/18539 With that, TPC-H sf1 with allowCpuFallback=false is at 19/22 — the remaining three (q1/q17/q22) Test gating is now the CudfTest tag per your earlier comment, and I deleted the q15-style test since the code it covered is gone. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
