Skip to content

Commit b450c3b

Browse files
committed
GH-51612: [C++] Fix list_view to list cast
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 GH-51613).
1 parent 9b34c4a commit b450c3b

8 files changed

Lines changed: 392 additions & 31 deletions

File tree

‎cpp/src/arrow/array/array_list_test.cc‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1049,6 +1049,35 @@ class TestListConversions : public ::testing::Test {
10491049
ASSERT_OK(result->ValidateFull());
10501050
AssertArraysEqual(*expected_list_wo_nulls, *result, /*verbose=*/true);
10511051
}
1052+
1053+
template <typename DestListType, typename SrcListViewType>
1054+
void DoTestListFromSlicedListView() {
1055+
using SrcListViewArrayClass = typename TypeTraits<SrcListViewType>::ArrayType;
1056+
using DestListArrayClass = typename TypeTraits<DestListType>::ArrayType;
1057+
auto list_view_type = std::make_shared<SrcListViewType>(int32());
1058+
auto list_type = std::make_shared<DestListType>(int32());
1059+
1060+
auto list_view =
1061+
ArrayFromJSON(list_view_type, "[[1, 2], [3], null, [], [4], [5, 6], null, [7]]");
1062+
1063+
auto check = [&](const Array& sliced, const std::shared_ptr<Array>& expected) {
1064+
ASSERT_OK_AND_ASSIGN(
1065+
auto result, DestListArrayClass::FromListView(
1066+
checked_cast<const SrcListViewArrayClass&>(sliced), pool_));
1067+
ASSERT_OK(result->ValidateFull());
1068+
AssertArraysEqual(*expected, *result, /*verbose=*/true);
1069+
};
1070+
1071+
// A slice whose element offset is not a multiple of 8, so the bitmap's bit
1072+
// offset and byte offset differ (GH-51613).
1073+
check(*list_view->Slice(1, 6),
1074+
ArrayFromJSON(list_type, "[[3], null, [], [4], [5, 6], null]"));
1075+
// A slice starting on a byte boundary, so that the bit offset is zero.
1076+
check(*list_view->Slice(2, 4), ArrayFromJSON(list_type, "[null, [], [4], [5, 6]]"));
1077+
// A slice whose only null is the last slot.
1078+
check(*list_view->Slice(0, 8),
1079+
ArrayFromJSON(list_type, "[[1, 2], [3], null, [], [4], [5, 6], null, [7]]"));
1080+
}
10521081
};
10531082

10541083
TEST_F(TestListConversions, ListViewFromList) {
@@ -1061,6 +1090,11 @@ TEST_F(TestListConversions, ListFromListView) {
10611090
this->DoTestListFromListView<LargeListType, LargeListViewType>();
10621091
}
10631092

1093+
TEST_F(TestListConversions, ListFromSlicedListView) {
1094+
this->DoTestListFromSlicedListView<ListType, ListViewType>();
1095+
this->DoTestListFromSlicedListView<LargeListType, LargeListViewType>();
1096+
}
1097+
10641098
// ----------------------------------------------------------------------
10651099
// Map tests
10661100

‎cpp/src/arrow/array/array_nested.cc‎

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -415,37 +415,8 @@ Result<std::shared_ptr<ArrayData>> ListFromListViewImpl(
415415
std::is_same<typename SrcListViewType::offset_type,
416416
typename DestListType::offset_type>::value,
417417
"Offset types between list type and list-view type are expected to match");
418-
using offset_type = typename DestListType::offset_type;
419-
using ListBuilderType = typename TypeTraits<DestListType>::BuilderType;
420-
421-
const auto& list_view_type =
422-
checked_cast<const SrcListViewType&>(*list_view_data->type);
423-
const auto& value_type = list_view_type.value_type();
424-
const auto list_type = std::make_shared<DestListType>(value_type);
425-
426-
ARROW_ASSIGN_OR_RAISE(auto sum_of_list_view_sizes,
427-
list_util::internal::SumOfLogicalListSizes(*list_view_data));
428-
ARROW_ASSIGN_OR_RAISE(std::shared_ptr<ArrayBuilder> value_builder,
429-
MakeBuilder(value_type, pool));
430-
RETURN_NOT_OK(value_builder->Reserve(sum_of_list_view_sizes));
431-
auto list_builder = std::make_shared<ListBuilderType>(pool, value_builder, list_type);
432-
RETURN_NOT_OK(list_builder->Reserve(list_view_data->length));
433-
434-
ArraySpan values{*list_view_data->child_data[0]};
435-
const auto* in_validity_bitmap = list_view_data->GetValues<uint8_t>(0);
436-
const auto* in_offsets = list_view_data->GetValues<offset_type>(1);
437-
const auto* in_sizes = list_view_data->GetValues<offset_type>(2);
438-
for (int64_t i = 0; i < list_view_data->length; ++i) {
439-
const bool is_valid =
440-
!in_validity_bitmap ||
441-
bit_util::GetBit(in_validity_bitmap, list_view_data->offset + i);
442-
const int64_t size = is_valid ? in_sizes[i] : 0;
443-
RETURN_NOT_OK(list_builder->Append(is_valid, size));
444-
RETURN_NOT_OK(value_builder->AppendArraySlice(values, in_offsets[i], size));
445-
}
446-
std::shared_ptr<ArrayData> list_array_data;
447-
RETURN_NOT_OK(list_builder->FinishInternal(&list_array_data));
448-
return list_array_data;
418+
return list_util::internal::ListFromListView(ArraySpan{*list_view_data},
419+
DestListType::type_id, pool);
449420
}
450421

451422
} // namespace

‎cpp/src/arrow/compute/kernels/scalar_cast_nested.cc‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "arrow/compute/kernels/scalar_cast_internal.h"
3131
#include "arrow/util/bitmap_ops.h"
3232
#include "arrow/util/int_util.h"
33+
#include "arrow/util/list_util.h"
3334
#include "arrow/util/logging_internal.h"
3435

3536
namespace arrow {
@@ -105,7 +106,37 @@ struct CastList {
105106
return Status::OK();
106107
}
107108

109+
/// \brief Cast a list-view array to a (large)list array
110+
///
111+
/// The values referenced by a list-view have to be gathered according to the
112+
/// views' offsets and sizes, so the views' offsets buffer cannot be reused as
113+
/// a list-offsets buffer (GH-51612).
114+
static Status ExecListView(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
115+
const CastOptions& options = CastState::Get(ctx);
116+
auto child_type = checked_cast<const DestType&>(*out->type()).value_type();
117+
118+
ARROW_ASSIGN_OR_RAISE(std::shared_ptr<ArrayData> list_data,
119+
list_util::internal::ListFromListView(
120+
batch[0].array, DestType::type_id, ctx->memory_pool()));
121+
122+
ARROW_ASSIGN_OR_RAISE(Datum cast_values, Cast(list_data->child_data[0], child_type,
123+
options, ctx->exec_context()));
124+
DCHECK(cast_values.is_array());
125+
126+
ArrayData* out_array = out->array_data().get();
127+
out_array->buffers[0] = std::move(list_data->buffers[0]);
128+
out_array->buffers[1] = std::move(list_data->buffers[1]);
129+
out_array->null_count.store(list_data->null_count.load());
130+
out_array->offset = list_data->offset;
131+
out_array->child_data.push_back(cast_values.array());
132+
return Status::OK();
133+
}
134+
108135
static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
136+
if constexpr (is_list_view(SrcType::type_id)) {
137+
return ExecListView(ctx, batch, out);
138+
}
139+
109140
const CastOptions& options = CastState::Get(ctx);
110141

111142
auto child_type = checked_cast<const DestType&>(*out->type()).value_type();

‎cpp/src/arrow/compute/kernels/scalar_cast_test.cc‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3616,6 +3616,135 @@ static void CheckListToList(const std::vector<std::shared_ptr<DataType>>& value_
36163616
}
36173617
}
36183618

3619+
TEST(Cast, ListViewToList) {
3620+
// Views may be out of order or overlapping, so the referenced values have to
3621+
// be gathered rather than reinterpreted (GH-51612).
3622+
auto values = ArrayFromJSON(int32(), "[1, 2, 3, 4]");
3623+
3624+
// Out of order and overlapping views.
3625+
auto offsets = ArrayFromJSON(int32(), "[2, 0, 1]");
3626+
auto sizes = ArrayFromJSON(int32(), "[2, 2, 1]");
3627+
ASSERT_OK_AND_ASSIGN(auto list_view,
3628+
ListViewArray::FromArrays(*offsets, *sizes, *values));
3629+
CheckCast(list_view, ArrayFromJSON(list(int32()), "[[3, 4], [1, 2], [2]]"));
3630+
// Same offset width, larger offset width, and a child type change.
3631+
CheckCast(list_view, ArrayFromJSON(large_list(int32()), "[[3, 4], [1, 2], [2]]"));
3632+
CheckCast(list_view, ArrayFromJSON(list(int64()), "[[3, 4], [1, 2], [2]]"));
3633+
CheckCast(list_view, ArrayFromJSON(large_list(float32()), "[[3, 4], [1, 2], [2]]"));
3634+
3635+
// The large_list_view source, narrowing as well as widening the offset width.
3636+
ASSERT_OK_AND_ASSIGN(
3637+
auto large_list_view,
3638+
LargeListViewArray::FromArrays(*ArrayFromJSON(int64(), "[2, 0, 1]"),
3639+
*ArrayFromJSON(int64(), "[2, 2, 1]"), *values));
3640+
CheckCast(large_list_view, ArrayFromJSON(large_list(int32()), "[[3, 4], [1, 2], [2]]"));
3641+
CheckCast(large_list_view, ArrayFromJSON(list(int32()), "[[3, 4], [1, 2], [2]]"));
3642+
3643+
// Views that happen to be contiguous, i.e. what a list array would look like.
3644+
ASSERT_OK_AND_ASSIGN(list_view, ListViewArray::FromArrays(
3645+
*ArrayFromJSON(int32(), "[0, 2, 4]"),
3646+
*ArrayFromJSON(int32(), "[2, 2, 0]"), *values));
3647+
CheckCast(list_view, ArrayFromJSON(list(int32()), "[[1, 2], [3, 4], []]"));
3648+
3649+
// Nulls.
3650+
{
3651+
auto list_view_w_nulls = MaskArrayWithNullsAt(list_view, {0});
3652+
CheckCast(list_view_w_nulls, ArrayFromJSON(list(int32()), "[null, [3, 4], []]"));
3653+
}
3654+
{
3655+
// A null view contributes no values, whatever size it declares.
3656+
ASSERT_OK_AND_ASSIGN(auto lv, ListViewArray::FromArrays(
3657+
*ArrayFromJSON(int32(), "[0, 2, 2]"),
3658+
*ArrayFromJSON(int32(), "[2, 2, 2]"), *values));
3659+
CheckCast(MaskArrayWithNullsAt(lv, {2}),
3660+
ArrayFromJSON(list(int32()), "[[1, 2], [3, 4], null]"));
3661+
}
3662+
3663+
// Sliced list-view.
3664+
{
3665+
ASSERT_OK_AND_ASSIGN(
3666+
auto lv, ListViewArray::FromArrays(*ArrayFromJSON(int32(), "[0, 2, 0, 1, 2, 0]"),
3667+
*ArrayFromJSON(int32(), "[2, 2, 2, 0, 2, 2]"),
3668+
*values));
3669+
CheckCast(lv->Slice(2, 3), ArrayFromJSON(list(int32()), "[[1, 2], [], [3, 4]]"));
3670+
CheckCast(lv->Slice(1, 4),
3671+
ArrayFromJSON(large_list(int32()), "[[3, 4], [1, 2], [], [3, 4]]"));
3672+
}
3673+
3674+
// Options pass through to the child cast.
3675+
{
3676+
ASSERT_OK_AND_ASSIGN(
3677+
auto lv, ListViewArray::FromArrays(
3678+
*ArrayFromJSON(int32(), "[0, 2]"), *ArrayFromJSON(int32(), "[2, 2]"),
3679+
*ArrayFromJSON(int64(), "[87654321, 0, 0, 0]")));
3680+
auto options = CastOptions::Safe(list(int16()));
3681+
CheckCastFails(lv, options);
3682+
options.allow_int_overflow = true;
3683+
CheckCast(lv, ArrayFromJSON(list(int16()), "[[32689, 0], [0, 0]]"), options);
3684+
}
3685+
}
3686+
3687+
TEST(Cast, ListViewToListEdgeCases) {
3688+
auto values = ArrayFromJSON(int32(), "[1, 2, 3, 4]");
3689+
3690+
auto make = [](const char* offsets, const char* sizes,
3691+
const std::shared_ptr<Array>& values) {
3692+
return ListViewArray::FromArrays(*ArrayFromJSON(int32(), offsets),
3693+
*ArrayFromJSON(int32(), sizes), *values);
3694+
};
3695+
3696+
// Zero-length list-view.
3697+
CheckCast(ArrayFromJSON(list_view(int32()), "[]"), ArrayFromJSON(list(int32()), "[]"));
3698+
CheckCast(ArrayFromJSON(large_list_view(int32()), "[]"),
3699+
ArrayFromJSON(large_list(int32()), "[]"));
3700+
3701+
// All views null, several with a non-zero declared size: no value is gathered.
3702+
{
3703+
ASSERT_OK_AND_ASSIGN(auto lv, make("[0, 2, 4]", "[2, 2, 2]", values));
3704+
CheckCast(MaskArrayWithNullsAt(lv, {0, 1, 2}),
3705+
ArrayFromJSON(list(int32()), "[null, null, null]"));
3706+
}
3707+
3708+
// Out-of-order/overlapping views combined with nulls.
3709+
{
3710+
ASSERT_OK_AND_ASSIGN(auto lv, make("[2, 0, 1]", "[2, 2, 1]", values));
3711+
CheckCast(MaskArrayWithNullsAt(lv, {1}),
3712+
ArrayFromJSON(list(int32()), "[[3, 4], null, [2]]"));
3713+
}
3714+
3715+
// A view of size zero interleaved with out-of-order views.
3716+
{
3717+
ASSERT_OK_AND_ASSIGN(auto lv, make("[1, 3, 0, 2]", "[0, 1, 4, 2]", values));
3718+
CheckCast(lv, ArrayFromJSON(list(int32()), "[[], [4], [1, 2, 3, 4], [3, 4]]"));
3719+
}
3720+
3721+
// The list-view's child array is itself sliced, i.e. has a non-zero offset.
3722+
{
3723+
auto sliced_values = ArrayFromJSON(int32(), "[9, 1, 2, 3, 4, 9]")->Slice(1, 4);
3724+
ASSERT_OK_AND_ASSIGN(auto lv, make("[2, 0, 1]", "[2, 2, 1]", sliced_values));
3725+
CheckCast(lv, ArrayFromJSON(list(int32()), "[[3, 4], [1, 2], [2]]"));
3726+
}
3727+
3728+
// Nested list-view, exercising the gather on a non-primitive child.
3729+
{
3730+
auto nested_values =
3731+
ArrayFromJSON(list(int32()), "[[10], [20, 21], [30, 31, 32], [40]]");
3732+
ASSERT_OK_AND_ASSIGN(auto lv, make("[1, 0, 2]", "[1, 2, 2]", nested_values));
3733+
CheckCast(lv, ArrayFromJSON(large_list(list(int64())),
3734+
"[[[20, 21]], [[10], [20, 21]], [[30, 31, 32], [40]]]"));
3735+
}
3736+
3737+
// A sliced list-view whose validity bitmap bit offset is non-zero.
3738+
{
3739+
auto lv = ArrayFromJSON(list_view(int32()),
3740+
"[[1, 2], [3], null, [], [4], [5, 6], null, [7], [8, 9]]");
3741+
CheckCast(lv->Slice(1, 7),
3742+
ArrayFromJSON(list(int32()), "[[3], null, [], [4], [5, 6], null, [7]]"));
3743+
CheckCast(lv->Slice(1, 7), ArrayFromJSON(large_list(int64()),
3744+
"[[3], null, [], [4], [5, 6], null, [7]]"));
3745+
}
3746+
}
3747+
36193748
TEST(Cast, ListToList) {
36203749
CheckListToList({int32(), float32(), int64()},
36213750
"[[0], [1], null, [2, 3, 4], [5, 6], null, [], [7], [8, 9]]");

‎cpp/src/arrow/util/list_util.cc‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,11 @@
1616
// under the License.
1717

1818
#include <cstdint>
19+
#include <memory>
1920
#include <vector>
2021

2122
#include "arrow/array/array_nested.h"
23+
#include "arrow/array/builder_base.h"
2224
#include "arrow/array/builder_nested.h"
2325
#include "arrow/array/data.h"
2426
#include "arrow/type.h"
@@ -190,8 +192,78 @@ int64_t SumOfListViewSizes(const ArraySpan& input) {
190192
return sum;
191193
}
192194

195+
/// \param list_view A LIST_VIEW or LARGE_LIST_VIEW array
196+
template <typename DestListType, typename SrcListViewType>
197+
Result<std::shared_ptr<ArrayData>> ListFromListViewImpl(const ArraySpan& list_view,
198+
MemoryPool* pool) {
199+
using src_offset_type = typename SrcListViewType::offset_type;
200+
using ListBuilderType = typename TypeTraits<DestListType>::BuilderType;
201+
202+
const auto& list_view_type = checked_cast<const SrcListViewType&>(*list_view.type);
203+
const auto& value_type = list_view_type.value_type();
204+
// The value type is carried over, so that callers may cast the child.
205+
auto list_type = std::make_shared<DestListType>(value_type);
206+
207+
ARROW_ASSIGN_OR_RAISE(auto sum_of_sizes, SumOfLogicalListSizes(list_view));
208+
ARROW_ASSIGN_OR_RAISE(std::shared_ptr<ArrayBuilder> value_builder,
209+
MakeBuilder(value_type, pool));
210+
RETURN_NOT_OK(value_builder->Reserve(sum_of_sizes));
211+
auto list_builder = std::make_shared<ListBuilderType>(pool, value_builder, list_type);
212+
RETURN_NOT_OK(list_builder->Reserve(list_view.length));
213+
214+
// The bitmap is bit-packed, so it must be read with the element offset
215+
// applied as a bit offset, not as the byte offset GetValues() would apply.
216+
const uint8_t* validity = list_view.buffers[0].data;
217+
const auto* offsets = list_view.GetValues<src_offset_type>(1);
218+
const auto* sizes = list_view.GetValues<src_offset_type>(2);
219+
const ArraySpan values{list_view.child_data[0]};
220+
for (int64_t i = 0; i < list_view.length; ++i) {
221+
const bool is_valid = !validity || bit_util::GetBit(validity, list_view.offset + i);
222+
// A null view contributes no values, whatever size it declares.
223+
const int64_t size = is_valid ? sizes[i] : 0;
224+
RETURN_NOT_OK(list_builder->Append(is_valid, size));
225+
RETURN_NOT_OK(value_builder->AppendArraySlice(values, offsets[i], size));
226+
}
227+
228+
std::shared_ptr<ArrayData> list_data;
229+
RETURN_NOT_OK(list_builder->FinishInternal(&list_data));
230+
return list_data;
231+
}
232+
193233
} // namespace
194234

235+
Result<std::shared_ptr<ArrayData>> ListFromListView(const ArraySpan& input,
236+
Type::type dest_type_id,
237+
MemoryPool* pool) {
238+
switch (input.type->id()) {
239+
case Type::LIST_VIEW:
240+
switch (dest_type_id) {
241+
case Type::LIST:
242+
return ListFromListViewImpl<ListType, ListViewType>(input, pool);
243+
case Type::LARGE_LIST:
244+
return ListFromListViewImpl<LargeListType, ListViewType>(input, pool);
245+
default:
246+
break;
247+
}
248+
break;
249+
case Type::LARGE_LIST_VIEW:
250+
switch (dest_type_id) {
251+
case Type::LIST:
252+
return ListFromListViewImpl<ListType, LargeListViewType>(input, pool);
253+
case Type::LARGE_LIST:
254+
return ListFromListViewImpl<LargeListType, LargeListViewType>(input, pool);
255+
default:
256+
break;
257+
}
258+
break;
259+
default:
260+
break;
261+
}
262+
return Status::TypeError("ListFromListView: expected a list-view array and a ",
263+
arrow::internal::ToString(dest_type_id),
264+
" destination type, got ", input.type->ToString());
265+
}
266+
195267
Result<std::pair<int64_t, int64_t>> RangeOfValuesUsed(const ArraySpan& input) {
196268
switch (input.type->id()) {
197269
case Type::LIST:

‎cpp/src/arrow/util/list_util.h‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#pragma once
1919

2020
#include <cstdint>
21+
#include <memory>
2122
#include <utility>
2223

2324
#include "arrow/array/data.h"
@@ -49,6 +50,20 @@ ARROW_EXPORT Result<std::pair<int64_t, int64_t>> RangeOfValuesUsed(
4950
/// \return The sum of all list or list-view sizes
5051
ARROW_EXPORT Result<int64_t> SumOfLogicalListSizes(const ArraySpan& input);
5152

53+
/// \brief Materialize a (large)list array from a list-view array
54+
///
55+
/// The returned array data has `dest_type_id`'s layout: freshly computed list
56+
/// offsets and a child array gathering the values referenced by every view, in
57+
/// list order. The value type is the input's, so that callers may cast the
58+
/// child afterwards.
59+
///
60+
/// \param input The input list-view array
61+
/// \param dest_type_id Either `Type::LIST` or `Type::LARGE_LIST`
62+
/// \param pool MemoryPool used for the allocations
63+
ARROW_EXPORT Result<std::shared_ptr<ArrayData>> ListFromListView(const ArraySpan& input,
64+
Type::type dest_type_id,
65+
MemoryPool* pool);
66+
5267
} // namespace internal
5368

5469
} // namespace list_util

0 commit comments

Comments
 (0)