ARROW-11394: [Rust] Tests for Slice & Concat #9339
Conversation
| let data = mutable.freeze(); | ||
| let array = StructArray::from(Arc::new(data)); | ||
|
|
||
| // Struct equality doesn't seem to work when using slices? |
There was a problem hiding this comment.
This has me suspicious of something not working. Specifically, using equality on the struct or the strings doesn't seem to work as I would expect.
There was a problem hiding this comment.
Related to #9211 . When we slice an array, we increase its offset. However, for StructArrays, I think that we also need to increase the offset of the childs (or something like that). :(
There was a problem hiding this comment.
That makes sense. It seems like it may also affect the string array. When I create the expected string directly instead of slicing it this test passes (see newest commit).
There was a problem hiding this comment.
I am sorry, I was not very specific. There are two different issues: for string arrays, binary, etc., I think that the issue is solved with #9211, via https://github.com/apache/arrow/pull/9211/files#diff-74d8df3798e724950c2eb5aae1a838fc2f2b9e35d89ace2627e8e7a64584c71fR91
For the Struct, we need another patch.
There was a problem hiding this comment.
Ah. That makes sense. Thanks for the clarification!
It's unrelated to this PR but I was also wondering about the implementation of extend for structs. For other types, it seems like it is relatively efficient (extending with an entire slice) but for the struct case it seems like it iterates over each row and extends a single row at a time. Is that an actual issue? Would addressing that require the changes to struct you mention?
Codecov Report
@@ Coverage Diff @@
## master #9339 +/- ##
==========================================
+ Coverage 81.89% 81.92% +0.03%
==========================================
Files 215 215
Lines 52988 53101 +113
==========================================
+ Hits 43392 43505 +113
Misses 9596 9596
Continue to review full report at Codecov.
|
jorgecarleitao
left a comment
jorgecarleitao
left a comment
There was a problem hiding this comment.
This looks good. Thanks a lot for adding these tests and fixing an error.
I would move the failing test to a description of an issue in Jira you did that already :) https://issues.apache.org/jira/browse/ARROW-11394
alamb
changed the title
|
Hi @bjchambers, I think that the issue pointed to by this PR is about a bug, this I do not think that this PR fixes it. Did you created an issue for this, or should I create one and re-link this PR to it? |
|
The issue pointed to is the bug I filed and the tests here were based on the test in the bug report. Why do you think this doesn't fix that bug? |
|
@bjchambers , I am really sorry, I do not know where my head was when I concluded that. You are obviously right. Let me merge this. |
|
Heh. No worries. Thanks for the quick review! |
3 participants
I took a stab at adding some tests for concat (and the underlying MutableArrayData) that cover the use case of the array having an offset. I had some problems with the assertions on the string array which makes me think this isn't fully fixed, but I wanted to put it up and see if anyone had suggestions for how to address this.