Add IPC truncation test case for StructArray #2083
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2083 +/- ##
=======================================
Coverage 83.68% 83.69%
=======================================
Files 224 224
Lines 58826 58855 +29
=======================================
+ Hits 49227 49256 +29
Misses 9599 9599
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| let record_batch = create_batch(); | ||
| let record_batch_slice = record_batch.slice(1, 2); |
There was a problem hiding this comment.
There is currently an odd hack in ArrayData::slice for StructArray that pushes down the offset to the child arrays. You will need to manually construct the ArrayData for the StructArray in order to properly test the case of a StructArray with a non-zero offset. See #1750 for more information
There was a problem hiding this comment.
ArrayData::Slice contains a special case for StructArray where it recurses the offset into its children. However, it preserves the offset on the parent ArrayData, in order for the validity buffer to work correctly.
I do see there is an special case, but not understand "it preserves the offset on the parent ArrayData".
let new_offset = self.offset + offset;
let new_data = ArrayData {
...
offset: new_offset,
...
};It updates new ArrayData with new offset, no?
There was a problem hiding this comment.
Yes, but it also applies the offset to the child data, which is the topic of that ticket. We have partially pushed down the offset, which at best is extremely confusing, but is likely just plain incorrect
There was a problem hiding this comment.
I see. You are basically concerned about "This makes it unclear if an offset should or should not be applied to the child_array". As the updated offset is applied on both parent array and child arrays, it is somehow obscure that whether we should take parent's offset when working with child arrays.
|
Benchmark runs are scheduled for baseline = a7181dd and contender = 32867e3. 32867e3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
Thanks @tustvold |
Which issue does this PR close?
Closes #2082.
Related to #2080.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?