HuaHuaY commented on code in PR #50976:
URL: https://github.com/apache/arrow/pull/50976#discussion_r3862246984
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -141,6 +142,139 @@ void AddListCast(CastFunction* func) {
DCHECK_OK(func->AddKernel(SrcType::type_id, std::move(kernel)));
}
+template <typename SrcType, typename DestType>
+struct CastListView {
+ using src_offset_type = typename SrcType::offset_type;
+ using dest_offset_type = typename DestType::offset_type;
+
+ static constexpr bool is_downcast = sizeof(src_offset_type) >
sizeof(dest_offset_type);
+
+ static bool IsContiguous(const ArraySpan& in_array) {
+ if (in_array.length == 0) return true;
+ const auto* offsets = in_array.GetValues<src_offset_type>(1);
+ const auto* sizes = in_array.GetValues<src_offset_type>(2);
+ for (int64_t i = 0; i < in_array.length - 1; ++i) {
+ if (offsets[i] + sizes[i] != offsets[i + 1]) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+ const CastOptions& options = CastState::Get(ctx);
+ auto child_type = checked_cast<const DestType&>(*out->type()).value_type();
+ const ArraySpan& in_array = batch[0].array;
+ ArrayData* out_array = out->array_data().get();
+
+ ARROW_ASSIGN_OR_RAISE(out_array->buffers[0],
+ GetOrCopyNullBitmapBuffer(in_array,
ctx->memory_pool()));
+
+ std::shared_ptr<ArrayData> values = in_array.child_data[0].ToArrayData();
+
+ const auto* offsets = in_array.GetValues<src_offset_type>(1);
+ const auto* sizes = in_array.GetValues<src_offset_type>(2);
+
+ if (IsContiguous(in_array)) {
Review Comment:
If we have a `list_view<int32>` array which's content is `[[10], null]`:
```
values = [10, 40000]
offsets = [0, 1]
sizes = [1, 1]
valid = [true, false]
```
When we cast the array into `list<int16>` at line 260, it will fail
unexpectedly.
```cpp
TEST(Cast, ListViewWithNullValues) {
auto values = ArrayFromJSON(int32(), "[10, 40000]");
auto offsets = ArrayFromJSON(int32(), "[0, 1]");
auto sizes = ArrayFromJSON(int32(), "[1, 1]");
ASSERT_OK_AND_ASSIGN(auto source,
ListViewArray::FromArrays(*offsets, *sizes, *values));
auto source_with_null = MaskArrayWithNullsAt(source, {1});
auto expected = ArrayFromJSON(list(int16()), "[[10], null]");
CheckCast(source_with_null, expected);
}
```
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -141,6 +142,136 @@ void AddListCast(CastFunction* func) {
DCHECK_OK(func->AddKernel(SrcType::type_id, std::move(kernel)));
}
+template <typename SrcType, typename DestType>
+struct CastListView {
+ using src_offset_type = typename SrcType::offset_type;
+ using dest_offset_type = typename DestType::offset_type;
+
+ static constexpr bool is_downcast = sizeof(src_offset_type) >
sizeof(dest_offset_type);
+
+ static bool IsContiguous(const ArraySpan& in_array) {
+ if (in_array.length == 0) return true;
+ const auto* offsets = in_array.GetValues<src_offset_type>(1);
+ const auto* sizes = in_array.GetValues<src_offset_type>(2);
+ for (int64_t i = 0; i < in_array.length - 1; ++i) {
+ if (offsets[i] + sizes[i] != offsets[i + 1]) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+ const CastOptions& options = CastState::Get(ctx);
+ auto child_type = checked_cast<const DestType&>(*out->type()).value_type();
+ const ArraySpan& in_array = batch[0].array;
+ ArrayData* out_array = out->array_data().get();
+
+ ARROW_ASSIGN_OR_RAISE(out_array->buffers[0],
+ GetOrCopyNullBitmapBuffer(in_array,
ctx->memory_pool()));
+
+ std::shared_ptr<ArrayData> values = in_array.child_data[0].ToArrayData();
+
+ const auto* offsets = in_array.GetValues<src_offset_type>(1);
+ const auto* sizes = in_array.GetValues<src_offset_type>(2);
+
+ if (IsContiguous(in_array)) {
+ // Zero-copy fast-path: shift offsets and slice child values
+ ARROW_ASSIGN_OR_RAISE(
+ out_array->buffers[1],
+ ctx->Allocate(sizeof(dest_offset_type) * (in_array.length + 1)));
+ auto* dest_offsets = out_array->GetMutableValues<dest_offset_type>(1);
+
+ src_offset_type start_offset = in_array.length > 0 ? offsets[0] : 0;
Review Comment:
I think it's best to check if `in_array.length == 0` separately. This avoids
having to write many `== 0` or `> 0` statements, improving code readability.
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -141,6 +142,139 @@ void AddListCast(CastFunction* func) {
DCHECK_OK(func->AddKernel(SrcType::type_id, std::move(kernel)));
}
+template <typename SrcType, typename DestType>
+struct CastListView {
+ using src_offset_type = typename SrcType::offset_type;
+ using dest_offset_type = typename DestType::offset_type;
+
+ static constexpr bool is_downcast = sizeof(src_offset_type) >
sizeof(dest_offset_type);
+
+ static bool IsContiguous(const ArraySpan& in_array) {
+ if (in_array.length == 0) return true;
+ const auto* offsets = in_array.GetValues<src_offset_type>(1);
+ const auto* sizes = in_array.GetValues<src_offset_type>(2);
+ for (int64_t i = 0; i < in_array.length - 1; ++i) {
+ if (offsets[i] + sizes[i] != offsets[i + 1]) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+ const CastOptions& options = CastState::Get(ctx);
+ auto child_type = checked_cast<const DestType&>(*out->type()).value_type();
+ const ArraySpan& in_array = batch[0].array;
+ ArrayData* out_array = out->array_data().get();
+
+ ARROW_ASSIGN_OR_RAISE(out_array->buffers[0],
+ GetOrCopyNullBitmapBuffer(in_array,
ctx->memory_pool()));
+
+ std::shared_ptr<ArrayData> values = in_array.child_data[0].ToArrayData();
+
+ const auto* offsets = in_array.GetValues<src_offset_type>(1);
+ const auto* sizes = in_array.GetValues<src_offset_type>(2);
+
+ if (IsContiguous(in_array)) {
+ // Zero-copy fast-path: shift offsets and slice child values
+ ARROW_ASSIGN_OR_RAISE(
+ out_array->buffers[1],
+ ctx->Allocate(sizeof(dest_offset_type) * (in_array.length + 1)));
+ auto* dest_offsets = out_array->GetMutableValues<dest_offset_type>(1);
+
+ src_offset_type start_offset = in_array.length > 0 ? offsets[0] : 0;
+ src_offset_type end_offset = 0;
+ if (in_array.length > 0) {
+ end_offset = offsets[in_array.length - 1] + sizes[in_array.length - 1]
- start_offset;
+ }
+
+ if (is_downcast && in_array.length > 0) {
+ if (end_offset > std::numeric_limits<dest_offset_type>::max()) {
+ return Status::Invalid("ListView too large to convert to List");
+ }
+ }
+
+ for (int64_t i = 0; i < in_array.length; ++i) {
+ dest_offsets[i] = static_cast<dest_offset_type>(offsets[i] -
start_offset);
+ }
+ if (in_array.length > 0) {
+ dest_offsets[in_array.length] =
static_cast<dest_offset_type>(end_offset);
+ } else {
+ dest_offsets[0] = 0;
+ }
+
+ if (in_array.length > 0) {
+ values = values->Slice(start_offset, dest_offsets[in_array.length]);
+ } else {
+ values = values->Slice(0, 0);
+ }
+ } else {
+ // Non-contiguous path: compute new offsets, build take indices, call
Take
+ ARROW_ASSIGN_OR_RAISE(
+ out_array->buffers[1],
+ ctx->Allocate(sizeof(dest_offset_type) * (in_array.length + 1)));
+ auto* dest_offsets = out_array->GetMutableValues<dest_offset_type>(1);
+
+ src_offset_type current_offset = 0;
+ dest_offsets[0] = 0;
+ for (int64_t i = 0; i < in_array.length; ++i) {
+ if (in_array.IsNull(i)) {
+ dest_offsets[i + 1] = static_cast<dest_offset_type>(current_offset);
+ } else {
+ current_offset += sizes[i];
+ dest_offsets[i + 1] = static_cast<dest_offset_type>(current_offset);
+ }
+ }
+
+ if (is_downcast) {
+ if (current_offset > std::numeric_limits<dest_offset_type>::max()) {
+ return Status::Invalid("ListView too large to convert to List");
+ }
+ }
+
+ Int64Builder builder(ctx->memory_pool());
+ RETURN_NOT_OK(builder.Reserve(current_offset));
+ for (int64_t i = 0; i < in_array.length; ++i) {
+ if (!in_array.IsNull(i)) {
+ src_offset_type start = offsets[i];
+ src_offset_type size = sizes[i];
+ for (src_offset_type j = 0; j < size; ++j) {
+ builder.UnsafeAppend(start + j);
+ }
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(std::shared_ptr<Array> take_indices,
builder.Finish());
+
+ // Call take function
+ ExecContext* exec_ctx = ctx->exec_context();
+ ARROW_ASSIGN_OR_RAISE(
+ Datum taken_values,
+ CallFunction("take", {MakeArray(values), take_indices}, exec_ctx));
+ DCHECK(taken_values.is_array());
Review Comment:
Can we use `ListViewArray::Flatten` and `LargeListViewArray::Flatten`
instead? `typename TypeTraits<SrcType>::ArrayType` may be helpful.
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -141,6 +142,139 @@ void AddListCast(CastFunction* func) {
DCHECK_OK(func->AddKernel(SrcType::type_id, std::move(kernel)));
}
+template <typename SrcType, typename DestType>
+struct CastListView {
+ using src_offset_type = typename SrcType::offset_type;
+ using dest_offset_type = typename DestType::offset_type;
+
+ static constexpr bool is_downcast = sizeof(src_offset_type) >
sizeof(dest_offset_type);
+
+ static bool IsContiguous(const ArraySpan& in_array) {
+ if (in_array.length == 0) return true;
+ const auto* offsets = in_array.GetValues<src_offset_type>(1);
+ const auto* sizes = in_array.GetValues<src_offset_type>(2);
+ for (int64_t i = 0; i < in_array.length - 1; ++i) {
+ if (offsets[i] + sizes[i] != offsets[i + 1]) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+ const CastOptions& options = CastState::Get(ctx);
+ auto child_type = checked_cast<const DestType&>(*out->type()).value_type();
+ const ArraySpan& in_array = batch[0].array;
+ ArrayData* out_array = out->array_data().get();
+
+ ARROW_ASSIGN_OR_RAISE(out_array->buffers[0],
+ GetOrCopyNullBitmapBuffer(in_array,
ctx->memory_pool()));
+
+ std::shared_ptr<ArrayData> values = in_array.child_data[0].ToArrayData();
+
+ const auto* offsets = in_array.GetValues<src_offset_type>(1);
+ const auto* sizes = in_array.GetValues<src_offset_type>(2);
+
+ if (IsContiguous(in_array)) {
+ // Zero-copy fast-path: shift offsets and slice child values
+ ARROW_ASSIGN_OR_RAISE(
+ out_array->buffers[1],
+ ctx->Allocate(sizeof(dest_offset_type) * (in_array.length + 1)));
+ auto* dest_offsets = out_array->GetMutableValues<dest_offset_type>(1);
+
+ src_offset_type start_offset = in_array.length > 0 ? offsets[0] : 0;
+ src_offset_type end_offset = 0;
+ if (in_array.length > 0) {
+ end_offset = offsets[in_array.length - 1] + sizes[in_array.length - 1]
- start_offset;
+ }
+
+ if (is_downcast && in_array.length > 0) {
+ if (end_offset > std::numeric_limits<dest_offset_type>::max()) {
+ return Status::Invalid("ListView too large to convert to List");
Review Comment:
I think the only possible scenario here is that a `LargeListView` is being
converted into a `List`. Should we specify the type in the error message?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
