You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "scott-routledge2 (via GitHub)" <gi...@apache.org> on 2025/11/19 01:40:41 UTC
[PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
scott-routledge2 opened a new pull request, #48171:
URL: https://github.com/apache/arrow/pull/48171
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
Please remove this line and the above text before creating your pull request.
### Rationale for this change
Casting Binary offset -> Binary offset types relies on ZeroCopyCastExec, which propagates the offset of the input to the output. This can lead to larger allocations than necessary when casting arrays with offsets.
See https://github.com/apache/arrow/issues/43660 and
https://github.com/apache/arrow/pull/43661 for more context.
### What changes are included in this PR?
Ensure output array has a small offset (it can still be non-zero since reusing the null bitmap requires in_offset % 8 == out_offset % 8)
### Are these changes tested?
Ran unit tests.
### Are there any user-facing changes?
**This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
**This PR contains a "Critical Fix".** (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2556567561
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
Thanks for the suggestions! I didn't slice the offset buffers originally because they are reallocated by `CastBinaryToBinaryOffsets` with the correct offset. Although, this would only be true for the case where the offset type changes, in the case where the offset stays the same then we would either have to slice here or call `ZeroCopyCastExec` like in the comment above?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2604883727
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,10 +306,32 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
+ // Start with a zero-copy cast, but change indices to the correct size and set validity
+ // bitmap and offset if needed.
RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
- return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
- ctx, input, out->array_data().get());
+ if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+
+ // Slice buffers to reduce allocation when casting the offsets buffer
Review Comment:
I think we can be more verbose about this comment - it might not be obvious for others to see how the allocation is reduced.
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -3400,6 +3400,34 @@ TEST(Cast, StringToString) {
}
}
+TEST(Cast, StringToStringWithOffset) {
+ // GH-43660: Check casting String Arrays with nonzero offset
+ for (auto from_type : {utf8(), large_utf8()}) {
+ for (auto to_type : {utf8(), large_utf8()}) {
+ for (int64_t offset : {3, 8, 10, 12}) {
+ auto input_with_nulls = R"([
+ "foo", null, "bar", null, "quu", "foo", "baz", "bar",
+ null, "bar", "baz", null
+ ])";
+
+ auto input_arr_with_nulls = ArrayFromJSON(from_type, input_with_nulls);
+ auto output_arr_with_nulls = ArrayFromJSON(to_type, input_with_nulls);
+ CheckCast(input_arr_with_nulls->Slice(offset),
+ output_arr_with_nulls->Slice(offset));
Review Comment:
Maybe with some slice-with-length cases as well?
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,10 +306,32 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
+ // Start with a zero-copy cast, but change indices to the correct size and set validity
+ // bitmap and offset if needed.
RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
- return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
- ctx, input, out->array_data().get());
+ if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+
+ // Slice buffers to reduce allocation when casting the offsets buffer
+ int64_t input_offset = input_arr->offset;
+ size_t input_offset_type_size = sizeof(typename I::offset_type);
+ if (output->null_count != 0 && output->buffers[0]) {
+ // Avoid reallocation of the validity buffer by allowing some padding bits
+ output->offset = input_arr->offset % 8;
Review Comment:
```suggestion
output->offset = input_offset % 8;
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #48171:
URL: https://github.com/apache/arrow/pull/48171#issuecomment-3663137682
@github-actions crossbow submit -g cpp -g python
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2543151622
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
Review Comment:
```suggestion
output->length = input_arr->length;
// output->offset is set below
output->SetNullCount(input_arr->null_count);
```
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
I believe you should also slice the offsets buffer when you change the array offset.
```suggestion
// slice buffers to reduce allocation when casting the offsets buffer
auto length = input_arr->length;
if (output->null_count != 0 && output->buffers[0]) {
// avoiding reallocation of the validity buffer by allowing some padding bits
output->offset = input_arr->offset % 8;
output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8, length);
} else {
output->offset = 0;
}
output->buffers[1] = SliceBuffer(output->buffers[1], output->offset, length);
```
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
Review Comment:
```suggestion
// binary/string arrays don't have child_data
```
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
+
Review Comment:
We need to make sure all cases are covered correctly here. `enable_if_t<is_base_binary_type<I>::value && is_base_binary_type<O>::value, Status>` covers all pairs of `Binary/LargeBinary/String/LargeString`.
`if constexpr (!I::is_utf8 && O::is_utf8) {` handles the UTF8 validation in case we are going from `{Binary, LargeBinary}` to `{String, LargeString}`.
The allocation issue is only a problem when going from 32 to 64 or 64 to 32, so you can shape your new code like this:
```cpp
if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
ArrayData* output = out->array_data().get();
// ...
if (output->buffers[0]) {
...
} else {
output->offset = 0;
}
return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
ctx, input, out->array_data().get());
} else {
return ZeroCopyCastExec(ctx, batch, out);
}
```
----
Useful diagram I made some time ago: https://gist.github.com/felipecrv/3c02f3784221d946dec1b031c6d400db
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2585769433
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
Sorry for the late reply. What you are saying makes sense, however, I think I am still a little confused about specifics of the slicing here. Wouldn't we want to slice the offset buffer with a different value than the validity buffer?
For example, if we are casting a slice that has length 8 and offset 8, we would slice the
validity buffer with a value of 1, and be left with a buffer of length 1 representing the 8 null bits for the elements in our casted slice. We would also slice the offsets buffer by 1, which would mean the buffer will have length 16*offset_size - 1 and be out of alignment.
Similarly in the case where we have `output->null_count == 0` (and buffer[0] != nullptr), we would slice the offset buffer by 8, leaving a buffer of length 8, but we would also slice the validity buffer by 8 , which would go out of bounds.
Wouldn't we want to slice the offsets buffer by `input_arr->offset` and the validity buffer by `input_arr->offset / 8`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2585769433
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
Sorry for the late reply. What you are saying makes sense, however, I think I am still a little confused about specifics of the slicing here. Wouldn't we want to slice the offset buffer with a different value than the validity buffer?
For example, if we are casting a slice that has length 8 and offset 8, we would slice the
validity buffer with a value of 1, and be left with a buffer of length 1 representing the 8 null bits for the elements in our casted slice. We would also slice the offsets buffer by 1, which would mean the buffer will have length 15 and be out of alignment.
Similarly in the case where we have `output->null_count == 0` (and buffer[0] != nullptr), we would slice the offset buffer by 8, leaving a buffer of length 8, but we would also slice the validity buffer by 8 , which would go out of bounds.
Wouldn't we want to slice the offsets buffer by `input_arr->offset` and the validity buffer by `input_arr->offset / 8`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2543220465
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
I believe you should also slice the offsets buffer when you change the array offset.
```suggestion
// slice buffers to reduce allocation when casting the offsets buffer
auto length = input_arr->length;
int64_t buffer_offset = 0;
if (output->null_count != 0 && output->buffers[0]) {
// avoiding reallocation of the validity buffer by allowing some padding bits
output->offset = input_arr->offset % 8;
buffer_offset = input_arr->offset / 8;
} else {
output->offset = 0;
buffer_offset = input_arr->offset;
}
output->buffers[0] = SliceBuffer(output->buffers[0], buffer_offset, length);
output->buffers[1] = SliceBuffer(output->buffers[1], buffer_offset, length);
```
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
Please review my suggestions and tell if something is weird. This stuff is hard.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2585769433
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
Sorry for the late reply. What you are saying makes sense, however, I think I am still a little confused about specifics of the slicing here. Wouldn't we want to slice the offset buffer with a different value than the validity buffer?
For example, if we are casting a slice that has length 8 and offset 8, we would slice the
validity buffer with a value of 1, and be left with a buffer of length 1 representing the 8 null bits for the elements in our casted slice. We would also slice the offsets buffer by 1, which would mean the buffer will have length 15 and be out of alignment.
Similarly in the case where we have `output->null_count == 0` (and buffer[0] != nullptr), we would slice the offset buffer by 8, leaving a buffer of length 8, but we would also slice the validity buffer by 8 , which would go out of bounds.
So, in case 1, wouldn't we want to slice the offsets buffer by `input_arr->offset` and the validity buffer by `input_arr->offset / 8`
And in case 2 we would want to slice the offsets buffer by `input_arr->offset` and the validity buffer by `input_arr->offset / 8` (if it's not null)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2585769433
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
Sorry for the late reply. What you are saying makes sense, however, I think I am still a little confused about specifics of the slicing here. Wouldn't we want to slice the offset buffer with a different value than the validity buffer?
For example, if we are casting a slice that has length 8 and offset 8, we would slice the
validity buffer with a value of 1, and be left with a buffer of length 1 representing the 8 null bits for the elements in our casted slice. We would also slice the offsets buffer by 1, which would mean the buffer will have length 17*offset_size - 1 and be out of alignment.
Similarly in the case where we have `output->null_count == 0` (and buffer[0] != nullptr), we would slice the offset buffer by 8, leaving a buffer of size 17*offset_size - 8 and we would also slice the validity buffer by 8 , which would go out of bounds.
Wouldn't we want to slice the offsets buffer by `input_arr->offset* sizeof(typename I::offset_type)` and the validity buffer by `input_arr->offset / 8`
Edit: I think the "offset" in SliceBuffer is the byte-offset as opposed to the logical offset.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2597683183
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,10 +305,33 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
- return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
- ctx, input, out->array_data().get());
+ if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ // output->offset is set below
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+
+ // Slice buffers to reduce allocation when casting the offsets buffer
+ int64_t offset = input_arr->offset;
+ size_t input_offset_type_size = sizeof(typename I::offset_type);
+ if (output->null_count != 0 && output->buffers[0]) {
+ // Avoid reallocation of the validity buffer by allowing some padding bits
+ output->offset = input_arr->offset % 8;
+ } else {
+ output->offset = 0;
+ }
+ if (output->buffers[0]) {
+ output->buffers[0] = SliceBuffer(output->buffers[0], offset / 8);
+ }
+ output->buffers[1] = SliceBuffer(output->buffers[1], offset * input_offset_type_size);
Review Comment:
The current code seems correct to me. I'm not sure what's the concern here.
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,10 +305,34 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
- return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
- ctx, input, out->array_data().get());
+ if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ // output->offset is set below
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ // binary/string arrays don't have child_data
+
+ // Slice buffers to reduce allocation when casting the offsets buffer
+ int64_t offset = input_arr->offset;
Review Comment:
More explicit naming to distinguish input vs. output.
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,10 +305,34 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
- return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
- ctx, input, out->array_data().get());
+ if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ // output->offset is set below
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ // binary/string arrays don't have child_data
+
+ // Slice buffers to reduce allocation when casting the offsets buffer
+ int64_t offset = input_arr->offset;
Review Comment:
```suggestion
int64_t input_offset = input_arr->offset;
```
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,10 +305,34 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
- return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
- ctx, input, out->array_data().get());
+ if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
Review Comment:
Shall we organize the logic as:
```
RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
// Change output offset and slice buffers correspondingly.
return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(ctx, input, out->array_data().get());
}
return Status::OK();
```
to reuse some logic already in `ZeroCopyCastExec`? (I also see other cast functions having similar code organization).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2585769433
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
Sorry for the late reply. What you are saying makes sense, however, I think I am still a little confused about specifics of the slicing here. Wouldn't we want to slice the offset buffer with a different value than the validity buffer?
For example, if we are casting a slice that has length 8 and offset 8, we would slice the
validity buffer with a value of 1, and be left with a buffer of length 1 representing the 8 null bits for the elements in our casted slice. We would also slice the offsets buffer by 1, which would mean the buffer will have length 17*offset_size - 1 and be out of alignment.
Similarly in the case where we have `output->null_count == 0` (and buffer[0] != nullptr), we would slice the offset buffer by 8, leaving a buffer of size 17*offset_size - 8 and we would also slice the validity buffer by 8 , which would go out of bounds.
Wouldn't we want to slice the offsets buffer by `input_arr->offset` and the validity buffer by `input_arr->offset / 8`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2586541152
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,10 +305,33 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
- return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
- ctx, input, out->array_data().get());
+ if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ // output->offset is set below
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+
+ // Slice buffers to reduce allocation when casting the offsets buffer
+ int64_t offset = input_arr->offset;
+ size_t input_offset_type_size = sizeof(typename I::offset_type);
+ if (output->null_count != 0 && output->buffers[0]) {
+ // Avoid reallocation of the validity buffer by allowing some padding bits
+ output->offset = input_arr->offset % 8;
+ } else {
+ output->offset = 0;
+ }
+ if (output->buffers[0]) {
+ output->buffers[0] = SliceBuffer(output->buffers[0], offset / 8);
+ }
+ output->buffers[1] = SliceBuffer(output->buffers[1], offset * input_offset_type_size);
Review Comment:
I believe in order for this slicing logic to be fully correct it needs to take into account `output->offset` when doing the slicing as well as the minimum size requirements, 0 out offset bytes, etc. So it would look something like
``` c++
int64_t offset_buffer_offset = (input_offset - output_offset) * offset_type_size
int64_t offset_buffer_length = (length + output_offset + 1) * offset_type_size
if (offset_buffer_length < minimum_size) {
// update the output->offset so that it's now == minimum size
}
// memset the first offset bytes to zero?
``` ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2566729261
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
But if you don't slice they get reallocated with the same length, no? Nevertheless, it becomes a creates a very weird relationship between this code and the function. Making it very non-obvious how it is correct.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #48171:
URL: https://github.com/apache/arrow/pull/48171#issuecomment-3664116366
The failures seem unrelated. I'm merging now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #48171:
URL: https://github.com/apache/arrow/pull/48171#issuecomment-3688147633
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 86166d5874555465e39d827c1a4dfa631a8c76c2.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/58832878783) has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2586541152
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,10 +305,33 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
- return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
- ctx, input, out->array_data().get());
+ if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ // output->offset is set below
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+
+ // Slice buffers to reduce allocation when casting the offsets buffer
+ int64_t offset = input_arr->offset;
+ size_t input_offset_type_size = sizeof(typename I::offset_type);
+ if (output->null_count != 0 && output->buffers[0]) {
+ // Avoid reallocation of the validity buffer by allowing some padding bits
+ output->offset = input_arr->offset % 8;
+ } else {
+ output->offset = 0;
+ }
+ if (output->buffers[0]) {
+ output->buffers[0] = SliceBuffer(output->buffers[0], offset / 8);
+ }
+ output->buffers[1] = SliceBuffer(output->buffers[1], offset * input_offset_type_size);
Review Comment:
I believe in order for this slicing logic to be fully correct it needs to take into account `output->offset` when doing the slicing as well as the minimum size requirements. So it would look something like
``` c++
int64_t offset_buffer_offset = (input_offset - output_offset) * offset_type_size
int64_t offset_buffer_length = (length + output_offset + 1) * offset_type_size
if (offset_buffer_length < minimum_size) {
// update the output->offset so that it's now == minimum size
}
```
?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #48171:
URL: https://github.com/apache/arrow/pull/48171#issuecomment-3644697665
Let's wait for some more while to see if @felipecrv has other comments. Then I'll merge. Thanks @scott-routledge2 !
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2604745630
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,10 +305,33 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
- return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
- ctx, input, out->array_data().get());
+ if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ // output->offset is set below
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+
+ // Slice buffers to reduce allocation when casting the offsets buffer
+ int64_t offset = input_arr->offset;
+ size_t input_offset_type_size = sizeof(typename I::offset_type);
+ if (output->null_count != 0 && output->buffers[0]) {
+ // Avoid reallocation of the validity buffer by allowing some padding bits
+ output->offset = input_arr->offset % 8;
+ } else {
+ output->offset = 0;
+ }
+ if (output->buffers[0]) {
+ output->buffers[0] = SliceBuffer(output->buffers[0], offset / 8);
+ }
+ output->buffers[1] = SliceBuffer(output->buffers[1], offset * input_offset_type_size);
Review Comment:
When I was testing the buffer slicing logic here (for example, casting a slice with length 3 and offset 1) I was seeing an error that looked like:
```
'datum.make_array()->ValidateFull()' failed with Invalid: Offsets buffer size (bytes): 16 isn't large enough for length: 3 and offset: 1
```
Since I think technically the offsets buffer needs to be big enough to hold `offset + length + 1` elements whereas the way this is written it will only hold `length + 1` many elements. However, because of the code structure here, this path isn't actually reachable since we always reallocate the offsets buffer with the correct size in `CastBinaryToBinaryOffsets` anyways.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 merged PR #48171:
URL: https://github.com/apache/arrow/pull/48171
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48171:
URL: https://github.com/apache/arrow/pull/48171#issuecomment-3663146462
Revision: 49298434a839a082d494c8eacd6323a8a829f4f0
Submitted crossbow builds: [ursacomputing/crossbow @ actions-6816eda394](https://github.com/ursacomputing/crossbow/branches/all?query=actions-6816eda394)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252795/job/58266850061)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253328/job/58266851625)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/20288254167/job/58266854039)|
|example-python-minimal-build-fedora-conda|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253057/job/58266850812)|
|example-python-minimal-build-ubuntu-venv|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253263/job/58266851395)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252958/job/58266850542)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253607/job/58266852310)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253879/job/58266853181)|
|test-conda-python-3.10|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253483/job/58266851915)|
|test-conda-python-3.10-hdfs-2.9.2|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253763/job/58266852695)|
|test-conda-python-3.10-hdfs-3.2.1|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253455/job/58266851885)|
|test-conda-python-3.10-pandas-1.3.4-numpy-1.21.2|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253130/job/58266850963)|
|test-conda-python-3.11|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253500/job/58266852006)|
|test-conda-python-3.11-dask-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/20288254185/job/58266854424)|
|test-conda-python-3.11-dask-upstream_devel|[](https://github.com/ursacomputing/crossbow/actions/runs/20288254074/job/58266853760)|
|test-conda-python-3.11-hypothesis|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252849/job/58266850182)|
|test-conda-python-3.11-pandas-latest-numpy-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253018/job/58266850708)|
|test-conda-python-3.11-spark-master|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253081/job/58266850857)|
|test-conda-python-3.12|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253493/job/58266852016)|
|test-conda-python-3.12-cpython-debug|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253239/job/58266851308)|
|test-conda-python-3.12-pandas-latest-numpy-1.26|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253949/job/58266853331)|
|test-conda-python-3.12-pandas-latest-numpy-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253017/job/58266850642)|
|test-conda-python-3.13|[](https://github.com/ursacomputing/crossbow/actions/runs/20288254221/job/58266854192)|
|test-conda-python-3.13-pandas-nightly-numpy-nightly|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253309/job/58266851553)|
|test-conda-python-3.13-pandas-upstream_devel-numpy-nightly|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252685/job/58266849769)|
|test-conda-python-3.14|[](https://github.com/ursacomputing/crossbow/actions/runs/20288254144/job/58266853961)|
|test-conda-python-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253984/job/58266853550)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253064/job/58266850804)|
|test-cuda-cpp-ubuntu-24.04-cuda-13.0.2|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253696/job/58266852498)|
|test-cuda-python-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253928/job/58266853271)|
|test-cuda-python-ubuntu-24.04-cuda-13.0.2|[](https://github.com/ursacomputing/crossbow/actions/runs/20288254181/job/58266854089)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252703/job/58266849764)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253338/job/58266851610)|
|test-debian-12-python-3-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253823/job/58266852930)|
|test-debian-12-python-3-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/20288254084/job/58266853750)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253767/job/58266852710)|
|test-fedora-42-python-3|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253464/job/58266851923)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252665/job/58266849774)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252765/job/58266849956)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253850/job/58266853071)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253963/job/58266853323)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252826/job/58266850081)|
|test-ubuntu-22.04-python-3|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253845/job/58266852939)|
|test-ubuntu-22.04-python-313-freethreading|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253867/job/58266853164)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252855/job/58266850268)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253640/job/58266852353)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253615/job/58266852335)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253170/job/58266851123)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252822/job/58266850099)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/20288253104/job/58266850909)|
|test-ubuntu-24.04-python-3|[](https://github.com/ursacomputing/crossbow/actions/runs/20288252674/job/58266849799)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2543142180
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
+
Review Comment:
We need to make sure all cases are covered correctly here. `enable_if_t<is_base_binary_type<I>::value && is_base_binary_type<O>::value, Status>` covers all pairs of `Binary/LargeBinary/String/LargeString`.
`if constexpr (!I::is_utf8 && O::is_utf8) {` handles the UTF8 validation in case we are going from `{Binary, LargeBinary}` to `{String, LargeString}`.
The allocation issue is only a problem when going from 32 to 64 or 64 to 32, so you can shape your new code like this:
```cpp
if constexpr (sizeof(typename I::offset_type) != sizeof(typename O::offset_type)) {
std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
ArrayData* output = out->array_data().get();
// ...
if (output->buffers[0]) {
...
return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
ctx, input, out->array_data().get());
} else {
return ZeroCopyCastExec(ctx, batch, out);
}
```
----
Useful diagram I made some time ago: https://gist.github.com/felipecrv/3c02f3784221d946dec1b031c6d400db
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types [arrow]
Posted by "scott-routledge2 (via GitHub)" <gi...@apache.org>.
scott-routledge2 commented on code in PR #48171:
URL: https://github.com/apache/arrow/pull/48171#discussion_r2585769433
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -304,8 +305,21 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
}
}
- // Start with a zero-copy cast, but change indices to expected size
- RETURN_NOT_OK(ZeroCopyCastExec(ctx, batch, out));
+ std::shared_ptr<ArrayData> input_arr = input.ToArrayData();
+ ArrayData* output = out->array_data().get();
+ output->length = input_arr->length;
+ output->SetNullCount(input_arr->null_count);
+ output->buffers = std::move(input_arr->buffers);
+ output->child_data = std::move(input_arr->child_data);
+
+ if (output->buffers[0]) {
+ // If reusing the null bitmap, ensure offset into the first byte is the same as input.
+ output->offset = input_arr->offset % 8;
+ output->buffers[0] = SliceBuffer(output->buffers[0], input_arr->offset / 8);
+ } else {
+ output->offset = 0;
+ }
Review Comment:
Sorry for the late reply. What you are saying makes sense, however, I think I am still a little confused about specifics of the slicing here. Wouldn't we want to slice the offset buffer with a different value than the validity buffer?
For example, if we are casting a slice that has length 8 and offset 8, we would slice the
validity buffer with a value of 1, and be left with a buffer of length 1 representing the 8 null bits for the elements in our casted slice. We would also slice the offsets buffer by 1, which would mean the buffer will have length 15 and be out of alignment.
Similarly in the case where we have `output->null_count == 0` (and buffer[0] != nullptr), we would slice the offset buffer by 8, leaving a buffer of length 8, but we would also slice the validity buffer by 8 , which would go out of bounds.
So, in case 1, wouldn't we want to slice the offsets buffer by `input_arr->offset - input_arr->offset%8` and the validity buffer by `input_arr->offset / 8`
And in case 2 we would want to slice the offsets buffer by `input_arr->offset` and the validity buffer by `input_arr->offset / 8` (if it's not null)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org