Skip to content

GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types #48171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zanmato1984 merged 15 commits into from
Dec 17, 2025
Merged

GH-43660: [C++][Compute] Avoid ZeroCopyCastExec when casting Binary offset -> Binary offset types #48171

Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e5b0d43
Avoid inheriting offset from input array during string cast
scott-routledge2 Nov 19, 2025
dfec3b6
remove iostream
scott-routledge2 Nov 19, 2025
6966d4f
remove include
scott-routledge2 Nov 19, 2025
71ed279
cleanup and add comment
scott-routledge2 Nov 19, 2025
49dfcb1
Merge branch 'main' into scott/cast_string
scott-routledge2 Dec 3, 2025
bbfd1e6
add test and address review comments
scott-routledge2 Dec 3, 2025
bb8c691
remove unused include
scott-routledge2 Dec 3, 2025
c8daf74
remove debugging prints
scott-routledge2 Dec 3, 2025
0b4c46b
add back comment
scott-routledge2 Dec 3, 2025
8006925
Merge branch 'main' into scott/cast_string
scott-routledge2 Dec 9, 2025
4f613e4
reuse some logic in ZeroCopyCastExec
scott-routledge2 Dec 9, 2025
e20f54b
fix offset buffer size
scott-routledge2 Dec 10, 2025
6fa4898
add back comment
scott-routledge2 Dec 10, 2025
48b0571
add slice with length test and make comment more verbose
scott-routledge2 Dec 11, 2025
4929843
Merge branch 'main' into scott/cast_string
scott-routledge2 Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions cpp/src/arrow/compute/kernels/scalar_cast_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include "arrow/array/array_base.h"
#include "arrow/array/builder_binary.h"
#include "arrow/buffer.h"
#include "arrow/compute/kernels/base_arithmetic_internal.h"
#include "arrow/compute/kernels/codegen_internal.h"
#include "arrow/compute/kernels/common_internal.h"
Expand Down Expand Up @@ -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;
}
Copy link
Contributor

@felipecrv felipecrv Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you should also slice the offsets buffer when you change the array offset.

Suggested change
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;
}
// 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);

Copy link
Contributor

@felipecrv felipecrv Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review my suggestions and tell if something is weird. This stuff is hard.

Copy link
Contributor Author

@scott-routledge2 scott-routledge2 Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@felipecrv felipecrv Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@scott-routledge2 scott-routledge2 Dec 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return CastBinaryToBinaryOffsets<typename I::offset_type, typename O::offset_type>(
ctx, input, out->array_data().get());
}
Expand Down
Loading