[GitHub] [arrow-rs] jhorstmann commented on a change in pull request #509: Fix for primitive and boolean take kernel for nullable indices with an offset

[GitBox <gi...@apache.org>]

jhorstmann commented on a change in pull request #509:
URL: https://github.com/apache/arrow-rs/pull/509#discussion_r661206212



##########
File path: arrow/src/compute/kernels/take.rs
##########
@@ -516,7 +522,7 @@ where
         nulls = match indices.data_ref().null_buffer() {
             Some(buffer) => Some(buffer_bin_and(
                 buffer,
-                0,
+                indices.offset(),
                 &null_buf.into(),
                 0,

Review comment:
       Yes, null_buf is newly constructed and initialized starting from 0, while the first buffer and offset pair are coming from the indices array which might have a non-0 offset.
   
   There was a proposal before to push the offsets down into all buffers instead of storing it in the array. That way we wouldn't need to care about which array a buffer originally belonged too. But if we do that we'd still need a better abstraction for the validity bitmap and only access it via (chunked) iterators. I'm also not sure whether such a change would have an affect on FFI usage.




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org