Repository navigation
GH-51612: [C++] Fix list_view to list cast - #51647
rajaryan2007 wants to merge 6 commits into
Conversation
|
|
|
@pitrou @felipecrv @kou could one of you take a look at this when you have a moment? It touches the list-view code ( Two points that probably need a committer's call:
|
CastList reused the view's offsets buffer as the list offsets and ignored the sizes, so out-of-order or overlapping views came out wrong. Gather the referenced values into a real list layout instead, reusing the shared helper in FromListView (which also fixes apacheGH-51613).
c2697d7 to
b450c3b
Compare
|
In Debug build, casting list_view<run_end_encoded<int32, extension>> aborts with Check failed: array.type->Equals(type_) (builder_run_end.cc:255). The builder has the storage type, but values has the extension type. Please fix it and add a test. |
…ypes ListFromListView builds the gathered values with a builder created from the storage type of the view's value type, because MakeBuilder does not support extension types. The appended slices, however, kept the original type, and a builder may check or inspect the type of the array it is given: the run-end encoded builder aborts (builder_run_end.cc) and the dictionary builder casts the type unchecked. Rewrite the type of the appended span tree to those same storage types, which the result's types are restored from afterwards.
A run-end encoded storage is the only storage type whose builder checks the type of the array it appends, so cover gathering through it: a plain run-end encoded child, an extension of one (the abort reported in apacheGH-51612), an extension nested in the value type of the runs, and one behind a struct, which the struct builder forwards to the run-end encoded field builder.
chrikrah
left a comment
There was a problem hiding this comment.
Nothing here blocks a merge.
# head 0cd374e27bff (base 9b34c4a6a), cmake -DCMAKE_BUILD_TYPE=Debug -DARROW_COMPUTE=ON -DARROW_BUILD_TESTS=ON, gcc 13
$ ./arrow-compute-scalar-cast-test; ./arrow-array-test; ./arrow-utility-test
[ PASSED ] 114 tests.
[ PASSED ] 1074 tests.
[ PASSED ] 854 tests.
# Cast.ListViewToListWithNestedRunEndEncodedChild covers the run_end_encoded<int32, extension> abort from the 10-28 comment, and passes in this Debug build.
# scalar_cast_nested.cc and array_nested.cc reverted to 9b34c4a6a, tests kept
$ ./arrow-compute-scalar-cast-test --gtest_filter=Cast.ListViewToList
data.cc:190: Check failed: (off) <= (length) Slice offset (1) greater than array length (0)
$ ./arrow-array-test
[ FAILED ] TestListConversions.ListFromSlicedListView
# not run: pyarrow tests/test_array.py, release build, benchmarks
non-blocking: the gather is stated; pitrou asked in #50976 for contiguous views sliced in the offsets loop, skipping values only null views reference. Tried that?
|
Could you split a fix for #51613 to another PR for easy to review?
@rajaryan2007 Can we keep zero-copy? |
Rationale for this change
Casting a
list_viewto alist(or across offset widths) incorrectly reused the offsets buffer and ignored thesizesbuffer. This caused out-of-bounds memory reads (crashes) and generated invalid data whenever views were overlapping or out-of-order.Are these changes tested?
Yes. Added and updated tests in C++ (scalar_cast_test.cc, list_util_test.cc, array_list_test.cc) and Python (test_array.py). Tests cover all offset-width combinations, null views, out-of-order/overlapping views, sliced inputs, zero-length edge cases, and nested list-views.
Are there any user-facing changes?
Yes.
Performance: list_view -> list casts are no longer zero-copy. The referenced values are now always gathered into a new child array. This is a necessary performance cost to guarantee correctness.
This PR includes breaking changes to public APIs.
ListArray::FromListView and LargeListViewArray::FromListView now return correct nulls for sliced inputs. This changes the output for users who may have been relying on the previous bugged behavior.
This PR contains a "Critical Fix".
Yes, this fixes a bug that produced incorrect and invalid data (negative/non-monotonic offsets that fail validation) and prevents Check failed: (off) <= (length) abort crashes caused by out-of-bounds reads.
Was AI used for this PR?
I used AI to help explain concepts, generate some inline code comments, and assist with writing and building tests.
PR code and description written by:
Reviewed before submission by: