mapleFU commented on code in PR #43190:
URL: https://github.com/apache/arrow/pull/43190#discussion_r1678010293
##########
cpp/src/arrow/array/concatenate.cc:
##########
@@ -133,26 +159,30 @@ Status ConcatenateOffsets(const BufferVector& buffers,
MemoryPool* pool,
for (size_t i = 0; i < buffers.size(); ++i) {
// the first offset from buffers[i] will be adjusted to values_length
// (the cumulative length of values spanned by offsets in previous buffers)
- RETURN_NOT_OK(PutOffsets<Offset>(*buffers[i], values_length,
- out_data + elements_length,
&(*values_ranges)[i]));
+ ARROW_ASSIGN_OR_RAISE(auto outcome, PutOffsets<Offset>(*buffers[i],
values_length,
+ out_data +
elements_length,
+
&(*values_ranges)[i]));
+ if (ARROW_PREDICT_FALSE(outcome != OffsetBufferOpOutcome::kOk)) {
Review Comment:
I know the reason why `RETURN_IF_NOT_OK_OUTCOME` not used but actually it's
a bitweird 🤔
##########
cpp/src/arrow/array/concatenate.cc:
##########
@@ -818,8 +937,31 @@ Result<std::shared_ptr<Array>> Concatenate(const
ArrayVector& arrays, MemoryPool
}
std::shared_ptr<ArrayData> out_data;
- RETURN_NOT_OK(ConcatenateImpl(data, pool).Concatenate(&out_data));
+ ErrorHints hints;
+ auto status = ConcatenateImpl(data, pool).Concatenate(&out_data, &hints);
+ if (!status.ok()) {
+ if (hints.suggested_cast) {
+ DCHECK(status.IsInvalid());
+ *out_suggested_cast = std::move(hints.suggested_cast);
Review Comment:
```
/// \param[out] out_suggested_cast if non-NULL, the function might set it to
a cast
/// suggestion that would allow concatenating
the arrays
/// without overflow of offsets (e.g. string
to
/// large_string)
```
So `out_suggested_cast` might be nullptr isn't it?
##########
cpp/src/arrow/array/concatenate_test.cc:
##########
@@ -661,14 +663,104 @@ TEST_F(ConcatenateTest, ExtensionType) {
});
}
+std::shared_ptr<DataType> LargeVersionOfType(const std::shared_ptr<DataType>&
type) {
+ switch (type->id()) {
+ case Type::BINARY:
+ return large_binary();
+ case Type::STRING:
Review Comment:
So view type would not meet the problem here? The new append data will point
to a new "buf. index" rather than concatenate togather in the previous buffer?
##########
cpp/src/arrow/array/concatenate.h:
##########
@@ -24,6 +24,23 @@
#include "arrow/util/visibility.h"
namespace arrow {
+namespace internal {
Review Comment:
why is this exported but internal? 🤔
--
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]
