ivanvankov commented on pull request #9647: URL: https://github.com/apache/arrow/pull/9647#issuecomment-792285533
Hi @jorgecarleitao, and thank you for the extended review and explanations. Now, regarding comments you have: - It does actually push just one byte per each 8 items. But here I've noticed a mistake :-) `null_buf.push(0)` actually pushes 4 bytes, so I'll change it to `null_buf.push(0u8)` to fix this. The invariants are still held though. - Regarding invariants I don't see anything that breaks them in my implementation. If you see anything please point to it. - The part with `unsafe` is for the case when iterator returns `None` as first item. Since we need to add to buffer a sequence of zeros of length `size` we need to know `size` to do that. But we still don't know `size` value, we need to get to the first `Some` to determine it. So, I only save count of `None` elements until iterator returns first `Some`, and don't add to buffer anything until then. When iterator stops we need to add `size * number of None` zeros in the beginning of buffer, and for that the section with `usafe` is added, just to copy from one position to other and put zeros. So, I need to prepend data, not to append, that's why `MutableBuffer::extend_from_slice` can't be used. I think, for more performance it would be better to prepend immediately after we determined `size`, not after iterator finished. - I'll add examples to documentation section for the functions. Feel free to add more remarks if you have any. Thanks. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
