felipecrv commented on code in PR #41561:
URL: https://github.com/apache/arrow/pull/41561#discussion_r1600327747
##########
cpp/src/arrow/chunk_resolver.cc:
##########
@@ -54,6 +54,52 @@ inline std::vector<int64_t> MakeChunksOffsets(const
std::vector<T>& chunks) {
offsets[chunks.size()] = offset;
return offsets;
}
+
+/// \pre all the pre-conditions of ChunkResolver::ResolveMany()
+/// \pre num_offsets - 1 <= std::numeric_limits<IndexType>::max()
+template <typename IndexType>
+void ResolveManyInline(size_t num_offsets, const int64_t* ARROW_RESTRICT
offsets,
+ int64_t n, const IndexType* ARROW_RESTRICT
logical_index_vec,
+ IndexType* ARROW_RESTRICT out_chunk_index_vec,
+ IndexType chunk_hint,
+ IndexType* ARROW_RESTRICT out_index_in_chunk_vec) {
+ const auto num_chunks = static_cast<IndexType>(num_offsets - 1);
+ // chunk_hint in [0, num_offsets) per the precondition.
+ for (int64_t i = 0; i < n; i++) {
+ const auto index = static_cast<uint64_t>(logical_index_vec[i]);
+ if (index >= static_cast<uint64_t>(offsets[chunk_hint]) &&
+ (chunk_hint == num_chunks ||
Review Comment:
Since before this PR, `ChunkResolver::Resolve` can handle out-of-bounds
inputs. I don't see why I would go about strengthening the pre-conditions now
at the risk of breaking existing callers.
> bisecting has a cost and perhaps you don't want to pay that cost over the
NULL entries of the indices array, only to ignore the results afterwards.
That's an efficiency concern/decision for the `Take` or other kernel
implementations. The pro here is that forgetting to filter out nulls doesn't
lead to out-of-memory access. Of the worst kind because most often values are
`0` when NULL, so it would be hard for an ASAN build to detect misuse.
I could pass a validity bitmap here at the cost of making the code
unnecessarily more complicated.
> updating the chunk hint based on the lookup of NULL indices leads to a
lower quality chunk hint.
I thought about this: it's no different from when a non-ordered list of
indices is passed.
In practice, the values for NULL array elements will all be `0` or values
that follow a regular pattern of the `indices` array but have been masked by a
validity bitmap with additional zero-bits.
Caring about this here would be premature optimization. The code is robust
against misuse. I can always add a version that takes an optional validity
bitmap in the future if that can be translated to better overall performance of
`Take` in real-world data.
--
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]
