Fix scalar as column view for null scalars - #22773
Conversation
📝 WalkthroughWalkthroughThis PR fixes a memory safety bug in cuDF's scalar-to-column-view conversion where null-aware binary operations incorrectly reinterpret scalar device bool validity data as a column bitmask, causing out-of-bounds reads. The fix explicitly materializes proper null masks for invalid scalars and omits masks for valid ones in both fixed-width and string_view specializations, validated by new null-aware fixed-point test cases. ChangesScalar null-mask handling in binary operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
bdice
left a comment
There was a problem hiding this comment.
Looks good to me.
The newly added tests surface the memcheck error when compute-sanitizer is run with
--rmm_mode=cuda.
Apologies, I should have suggested this earlier. I hope this didn't take too long to discover. OOB accesses are often reproducible only with the cuda_memory_resource. (Which is why we set it in our sanitizer tests.)
| !s.is_valid(stream)); | ||
| return std::pair{col_v, std::unique_ptr<column>(nullptr)}; | ||
|
|
||
| // Valid scalar needs no null mask |
There was a problem hiding this comment.
Just a reminder that we need to be careful with the data model here. A non-nullable column and a nullable column with valid data are not identical states. They can round-trip differently to/from various file formats.
If I recall correctly, cudf scalars don't distinguish "valid" from "non-nullable" states today, and thankfully this doesn't have much impact on I/O because we only read/write columns and not scalars, but it's important to keep that in mind.
No action needed, just want to build awareness.
There was a problem hiding this comment.
A non-nullable column and a nullable column with valid data are not identical states. They can (and do) round-trip differently to/from various file formats
Learned this the hard way back when I was a new hire while working on round trips with Arrow 😄
There was a problem hiding this comment.
If I recall correctly, cudf scalars don't distinguish "valid" from "non-nullable" states today, and thankfully this doesn't have much impact on I/O because we only read/write columns and not scalars, but it's important to keep that in mind.
Yup, this makes sense. Good for a temporary column view state but not when writing/reading.
I had just asked Claude to write the smallest possible repro (the test in the PR) and run it under compute-sanitizer. This was the first thing it did when the test didn't fail. All this happened while I was working on something else 😄 |
simoneves
left a comment
There was a problem hiding this comment.
LGTM
Tested in Velox
All decimal tests run clean under compute-sanitizer --tool memcheck
|
/merge |
…snapshot] Backport of the effective diff of rapidsai/cudf PR NVIDIA#23077 ("Prepend row index column in Parquet reader"), which was still OPEN at the time this branch was created. This captures the PR's net change as a single squashed commit computed as the diff between its merge-base with main and its head: merge-base: 52d322c head: 2574b9d Applied on top of the 26.06.01 pin + NVIDIA#22773 + NVIDIA#22879 backport. NOTE: This is a PRE-MERGE snapshot; re-sync to the squashed merge commit once NVIDIA#23077 lands upstream. Unrelated main-only tests present as diff context (MismatchedSchema*) were intentionally excluded.
Description
Closes #22757. Supersedes #22759.
This PR fixes
scalar_as_column_viewto handle null input input scalar by allocating a single-element bitmask for them (kept alive using the aux column). Valid scalars are still directly just converted to column views withnullptras their nullmask.The newly added tests surface the
memcheckerror whencompute-sanitizeris run with--rmm_mode=cuda.Credits: @simoneves for the original PR.
Checklist