feat(cudf): Restore Spark xxhash64 and runtime Bloom filters - #102
feat(cudf): Restore Spark xxhash64 and runtime Bloom filters#102sperlingxx wants to merge 2 commits into
Conversation
- Register xxhash64_with_seed and might_contain in the Spark cuDF expression adapter. - Add reduce-only bloom_filter_agg support for raw and intermediate/final aggregation. - Cover expression selection, xxhash64 parity, might_contain, and Bloom-filter aggregation. - Include the CudfReduce parenthesis correction required for GCC compilation. - Fuse 34d5c22 and 2842252 on dev 28798f6.
- Reject multi-column xxhash64_with_seed regardless of CPU fallback because cuDF does not implement Spark iterative seed chaining. - Preserve the single-column GPU path required by the Q20 runtime Bloom-filter workflow. - Cover supported single-column and rejected multi-column expression selection. Fixes #101
sperlingxx
changed the title
|
|
||
| std::vector<int64_t> hostKeys(static_cast<size_t>(numRows)); | ||
| if (numRows > 0) { | ||
| copyKeysToHost(inputView, hostKeys, stream); |
There was a problem hiding this comment.
Plz add some comments why we need to copy keys to host.
|
Do we have some perf number before vs. after? B/c previously we skipped bloom filter and use some plan optimizer avoiding bloom filter. |
| // seed-chaining (rapidsai/cudf#21720). Always reject this shape so callers | ||
| // either use CPU fallback or fail closed instead of returning wrong hashes. | ||
| const bool hasMultipleDataColumns = expr->inputs().size() > 2; | ||
| return !hasMultipleDataColumns; |
There was a problem hiding this comment.
[Blocking] Please gate the single-column path by input type (or normalize the input before hashing). The registered signature accepts any, but libcudf hashes the physical storage width while Spark widens TINYINT, SMALLINT, and BOOLEAN to 32 bits; libcudf also preserves the sign bit of -0.0, whereas Spark normalizes it to +0.0. For example, with seed 42, hashing TINYINT(1) as one byte yields 6668291691252061002, while Spark's 32-bit semantics yield -6698625589789238999. These shapes currently pass canEvaluate() and silently return incorrect hashes. Please restrict selection to parity-proven types or implement Spark-compatible widening/normalization, with coverage for every advertised type.
| bloomIsNull_ = true; | ||
| return; | ||
| } | ||
| auto serialized = bloomValue->as<SimpleVector<StringView>>()->valueAt(0); |
There was a problem hiding this comment.
[Blocking] Please validate the serialized value before passing it to BloomFilterView/mayContain. The CPU implementation checks serialized->size() >= BloomFilterView::kSerializedHeaderSize; this path stores any non-null literal without that check. A short value beginning with the valid version byte (for example, a one-byte 0x01) makes BloomFilterView read the four-byte size past the buffer, which is undefined behavior instead of the CPU path's user error. Please add the same minimum-header check here and a malformed-literal test.
3 participants
Summary
Restore the Spark cuDF expression and aggregation paths required by runtime Bloom filters:
xxhash64_with_seed;might_containfor serialized Spark/Velox Bloom filters;bloom_filter_aggfor raw and intermediate/final aggregation;The Bloom-filter probe and aggregation implementations deliberately use host copies and synchronization for compatibility. This PR does not claim a GPU-native performance implementation.
Why
Current
devfails TPC-H Q20 before execution when CPU fallback is disabled becausexxhash64_with_seedcannot be replaced. Ferdinand's July runtime already contained this support; it was lost from the current development line during a later upstream merge.Validation
git diff --checkpasses at668f0bc6ca37d1e8456c8389b347735040a1fb37.48615d8326b7077586137374c7e56d3611ce40d3compiled in NVL72 Slurm/Pyxis build job11596, and the bundle passedcheck-deployqualification.11606removed the prior hard replacement failure; Q20 completed all three attempts in 4.745 / 4.717 / 4.702 seconds.The runtime artifact predates the final fail-closed selector commit, but Q20 uses the unchanged single-column path. Result comparison was disabled in job
11606, so a CPU-oracle correctness comparison and performance qualification remain separate gates.AI disclosure
Generated-by: Codex (GPT-5)
Fixes #101