Repository navigation
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
fce680d to
0b590c7
Compare
Periecle
changed the title
| if (indices_may_have_nulls && | ||
| !bit_util::GetBit(indices.buffers[0].data, indices.offset + out_i)) { |
There was a problem hiding this comment.
I think you can use VisitTwoBitBlocksVoid which will be more efficient than individual calls to GetBit for each item.
| if (validity_buf->size() > 0) { | ||
| std::memset(validity_buf->mutable_data(), 0, validity_buf->size()); | ||
| } |
There was a problem hiding this comment.
AllocateEmptyBitmap should ensure that the bitmap is already zero-filled, IIRC.
| for (const auto& data_buffer : data_buffers) { | ||
| buffers.push_back(data_buffer); | ||
| } |
There was a problem hiding this comment.
This is going to keep alive all data buffers from the input, but it would be nice to only keep alive those that are actually used by the output.
Depending on the outcome from #50172 we may replace the unused buffers with NULL slots, or with zero-sized buffers (or we can remap the buffer indices, which is going to be more costly).
| if (out_validity != nullptr) { | ||
| bit_util::SetBit(out_validity, out_i); | ||
| } |
There was a problem hiding this comment.
OptionalBitmapAnd would probably be much more efficient than settings output validity bits individually.
|
Also @zanmato1984 might be interested as well. |
github-actions
Bot
added
awaiting committer review
zanmato1984
left a comment
zanmato1984
left a comment
There was a problem hiding this comment.
LGTM overall. I think this is appropriately scoped, and I don’t see anything else that needs to change from my side.
Could you still address @pitrou’s earlier comment before merging? I’d prefer we settle that point explicitly.
github-actions
Bot
added
awaiting committer review
Use VisitBitBlocksVoid to traverse index validity in blocks. Take gathers source validity through arbitrary (possibly repeated) indices, so VisitTwoBitBlocksVoid cannot directly combine the two input bitmaps. When source values are non-null, use OptionalBitmapAnd to reuse or realign the index bitmap. Otherwise gather validity with FirstTimeBitmapWriter, which avoids individual output-bitmap read/modify/write operations and a temporary gathered bitmap plus a second AND pass. AllocateBitmap suffices; remove redundant bitmap initialization and initialize only null view slots. Track payload buffers referenced by valid selected non-inline views. Preserve their indices with parent-free zero-sized Buffer objects for unused interior slots, and omit trailing unused slots. Never emit null variadic Buffer slots: the C Data Interface clarification in apache#50255 does not override the C++ validation requirement added in apache#50166. Add regressions for actual payload release, both view types, empty and all-null selections, retained buffer identity, C Data round trips, plain and run-end encoded filtering, and sliced/permuted validity across all eight integer index types. Null index payloads contain out-of-range values to ensure they are never dereferenced. Validation: - New Take and Filter retention regressions fail before this change. - Debug arrow-compute-vector-selection-test: 186/186 passed. - Debug arrow-compute-scalar-cast-test: 107/107 passed. - clang-format 18.1.8, cpplint 1.6.1, and git diff --check passed. - Complete PR diff applies to upstream main at 5ef3c52. AI assistance: Codex investigated the review, implemented this follow-up, wrote the regressions, reviewed the diff, and ran the checks above.
|
@pitrou and @zanmato1984. I fixed pitrou comments, can you take another look into that? |
3 participants
Rationale for this change
binary_viewandstring_viewarrays were not fully supported by selection kernels. This also blocked dictionary decoding for dictionaries with view-type values.What changes are included in this PR?
BINARY_VIEWandSTRING_VIEW.Are these changes tested?
Yes.
pre-commit run cpp --files <changed files>cmake --build cpp/build --target arrow-compute-vector-selection-testAre there any user-facing changes?
Yes.
binary_viewandstring_viewarrays are now supported by the affected selection and dictionary decode paths.Partially addresses GH-39634
Closes GH-43010