You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/11/04 18:37:01 UTC

[GitHub] [arrow] carols10cents commented on pull request #8590: ARROW-10042: [Rust] Only compare BufferData's data for equality

carols10cents commented on pull request #8590:
URL: https://github.com/apache/arrow/pull/8590#issuecomment-721903270


   If I remove the `impl PartialEq for BufferData` entirely, compiling then complains that the `#[derive(PartialEq)]` on `Buffer` can't work. If I remove the derived `PartialEq` on `Buffer`, I then get compilation errors in places such as:
   
   - https://github.com/apache/arrow/blob/913cd7612aebd235eaa35bf8b344448e281b2cbe/rust/arrow/src/array/data.rs#L264 the new-ish `compare_buffer_regions` function that I think is relevant to [this discussion](https://github.com/apache/arrow/pull/8541/files#r516989210)
   - https://github.com/apache/arrow/blob/5355f7cbc031b2dd4dcdf55aa15cb5c0ef80bea4/rust/arrow/src/array/array.rs#L2429 and a bunch of other tests in that file
   - https://github.com/apache/arrow/blob/913cd7612aebd235eaa35bf8b344448e281b2cbe/rust/arrow/src/array/builder.rs#L2705 and a bunch of other tests in that file
   - https://github.com/apache/arrow/blob/913cd7612aebd235eaa35bf8b344448e281b2cbe/rust/arrow/src/array/union.rs#L656-L659 and a bunch of other tests in that file 
   - A bunch of tests in arrow/src/buffer.rs of course
   - https://github.com/apache/arrow/blob/913cd7612aebd235eaa35bf8b344448e281b2cbe/rust/arrow/src/compute/kernels/cast.rs#L1365-L1368
   - https://github.com/apache/arrow/blob/913cd7612aebd235eaa35bf8b344448e281b2cbe/rust/arrow/src/compute/kernels/filter.rs#L1117-L1120 and a bunch of other tests in that file
   - https://github.com/apache/arrow/blob/913cd7612aebd235eaa35bf8b344448e281b2cbe/rust/arrow/src/compute/util.rs#L241-L244 and a bunch of other tests in that file
   
   I'm not sure enough of those cases whether they do expect the `capacity` to be equal and mean this `PartialEq` should stay as-is, or if these cases are being tested at too low a level and should be checking `ArrayData` instead... please let me know what you all think!


----------------------------------------------------------------
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:
users@infra.apache.org