fix(arrow): deep-copy the right rows from a sliced boolean column - #9021
Open
MaxFreedomPollard wants to merge 1 commit into
Open
fix(arrow): deep-copy the right rows from a sliced boolean column#9021MaxFreedomPollard wants to merge 1 commit into
MaxFreedomPollard wants to merge 1 commit into
Conversation
deep_copy_array_data_sliced called mutable.extend(0, data.offset(), data.offset() + data.len()). MutableArrayData::extend already indexes from the source array's first logical element: ArrayData::buffer hands the value extenders a slice starting at self.offset, and the null-bit extender adds nulls.offset() itself. Passing the offset in as start applies it twice, so the copy begins offset elements past the slice. Most arrays hide this because to_data() folds a slice into the buffer pointer and leaves ArrayData::offset at 0. A boolean array cannot, since a bit offset has no byte pointer to fold into, so From<BooleanArray> for ArrayData sets .offset(array.values.offset()) and a sliced boolean column keeps a non-zero offset. Its deep copy comes back shifted by that many bits, values and validity alike. Two public entry points reach it: RecordBatchExt::shrink_to_fit calls deep_copy_batch_sliced directly, and rechunk_stream_by_size_deep_copy deep-copies every slice it produces, which is how HardCapBatchSizeExec caps batch sizes ahead of a DataFusion sort. Copy from 0 instead, and cover it with a sliced boolean array that carries a non-zero ArrayData::offset.
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The fix restores the logical-slice contract: MutableArrayData::extend receives 0..len, so non-zero-offset boolean values and validity are copied once into compact buffers. The regression test covers the offset case, and the shared copy path remains correct for existing array types.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
deep_copy_array_data_slicedinrust/lance-arrow/src/deepcopy.rscopied from the wrong place:mutable.extend(0, data.offset(), data.offset() + data.len()).MutableArrayData::extendalready indexes from the source array's first logical element.ArrayData::bufferhands the value extenders a slice that starts atself.offset, and the null-bit extender addsnulls.offset()itself. Passing the offset in asstartapplies it a second time, so the copy beginsoffsetelements past the slice.Most arrays hide this, because
to_data()folds a slice into the buffer pointer and leavesArrayData::offsetat 0. A boolean array cannot: a bit offset has no byte pointer to fold into, soFrom<BooleanArray> for ArrayDatasets.offset(array.values.offset())and a sliced boolean column keeps a non-zero offset. Its deep copy comes back shifted by that many bits, values and validity alike.Two public entry points reach it.
RecordBatchExt::shrink_to_fitis a direct call intodeep_copy_batch_sliced. Andrechunk_stream_by_size_deep_copydeep-copies every slice it produces, which is howHardCapBatchSizeExeccaps batch sizes ahead of a DataFusion sort, so a large batch with a boolean column comes back through that node with the wrong booleans and no error.The fix is to copy from 0. The regression test slices a boolean array that has nulls at offset 3, asserts the slice really does carry
ArrayData::offset == 3, and compares the copy against the slice element by element.Verification, run from
rust/on 1.97.0:cargo test -p lance-arrowpasses, 102 unit tests and 6 doctests. Reverting just the one-line fix and keeping the test makestest_deep_copy_array_sliced_boolean_keeps_offsetfail withleft: [Some(true), Some(false), Some(true), Some(true)]againstright: [Some(true), Some(true), Some(false), Some(true)], while the five deepcopy tests that were already there still pass, which is why this went unnoticed.cargo clippy -p lance-arrow --all-targets --all-features -- -D warningsis clean, and so iscargo fmt --all -- --check.