aaron-seq commented on PR #50546: URL: https://github.com/apache/arrow/pull/50546#issuecomment-5101121120
Thanks @pitrou, all five points addressed in da10b42. **`CountAndNotSetBits`** — added to `arrow/util/bitmap_ops.h`/`.cc` (same `BinaryBitBlockCounter` shape as `CountAndSetBits`, using `NextAndNotWord()`), and the kernel now calls it directly instead of subtracting `CountAndSetBits` from `CountSetBits`. Added `BitUtilTests.TestCountAndNotSetBits` covering unaligned offsets on both sides, zero length, and the `(x & y) + (x & ~y) == x` identity. **Bitmap-less children** — you're right, I had this backwards. `GetNullCount()` only counts physical nulls, so union and run-end encoded report 0 and never reach the check at all. NullType is the only type that gets there without a validity buffer, so that branch is now just part of the rejection condition, with the comment rewritten to say so. **Test construction** — the nested-nullability inputs are now built with `StructArray::Make` from an explicit `"a"` child plus explicit validity bitmaps at each level, so where `"a"` physically holds nulls is pinned down by the test rather than being an artifact of `ArrayFromJSON`. This also caught that the sliced test wasn't exercising what its comment claimed: the `[3, 5)` slice had no null in `"a"` at all, so it was passing via the `GetNullCount() == 0` early exit rather than through the masked-null path. Fixed the fixture so it actually hits it. **NullType test comment** — rewritten; it no longer claims REE/union are rejected, and notes they report no physical nulls so they're unaffected. **Zero-length input** — added on both paths. It succeeds: an empty slice reports no nulls, so the check is skipped. Worth noting the NullType case is the interesting one — `src->Slice(1, 1)` is rejected but `src->Slice(1, 0)` succeeds, since `ArrayData::Slice` carries `null_count = len` for an all-null array. Also stripped trailing whitespace in `test_compute.py`, which was the autopep8 lint failure. The two red R jobs are unrelated — both are `R CMD check` failing on a Bioconductor index fetch (`cannot open URL '.../PACKAGES'`), with `FAIL 0` on the actual test suites. One thing I left alone pending the ML thread: the check runs per cast level, so if the outer element is null but the inner struct is *physically* valid there with a null leaf, the recursive inner cast still rejects it — it has no view of the outer bitmap. I didn't add a test asserting that either way, since it seems to be exactly what's under discussion. -- 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]
