bkietz commented on code in PR #43190:
URL: https://github.com/apache/arrow/pull/43190#discussion_r1676445852
##########
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:
+ return large_utf8();
+ case Type::LIST:
+ return large_list(static_cast<const ListType&>(*type).value_type());
+ case Type::LIST_VIEW:
+ return large_list_view(static_cast<const
ListViewType&>(*type).value_type());
+ case Type::LARGE_BINARY:
+ case Type::LARGE_STRING:
+ case Type::LARGE_LIST:
+ case Type::LARGE_LIST_VIEW:
+ return type;
+ default:
+ Unreachable();
+ break;
+ }
+}
+
+std::shared_ptr<DataType> fixed_size_list_of_1(std::shared_ptr<DataType> type)
{
+ return fixed_size_list(std::move(type), 1);
+}
+
TEST_F(ConcatenateTest, OffsetOverflow) {
- auto fake_long = ArrayFromJSON(utf8(), "[\"\"]");
- fake_long->data()->GetMutableValues<int32_t>(1)[1] =
+ using TypeFactory = std::shared_ptr<DataType> (*)(std::shared_ptr<DataType>);
+ static const std::vector<TypeFactory> kNestedTypeFactories = {
+ list, large_list, list_view, large_list_view, fixed_size_list_of_1,
+ };
+
+ auto* pool = default_memory_pool();
+ std::shared_ptr<DataType> suggested_cast;
+ for (auto& ty : {binary(), utf8()}) {
+ auto large_ty = LargeVersionOfType(ty);
+
+ auto fake_long = ArrayFromJSON(ty, "[\"\"]");
+ fake_long->data()->GetMutableValues<int32_t>(1)[1] =
+ std::numeric_limits<int32_t>::max();
+ // XXX: since the data fake_long claims to own isn't there, this would
Review Comment:
At some point, it'd be nice to have a utility which produces valid, huge
blocks of memory that is virtual enough to be quick to allocate and deallocate
as long as it is not accessed. For now it should be fine to rely on valgrind to
validate this
##########
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:
+ return large_utf8();
+ case Type::LIST:
+ return large_list(static_cast<const ListType&>(*type).value_type());
+ case Type::LIST_VIEW:
+ return large_list_view(static_cast<const
ListViewType&>(*type).value_type());
+ case Type::LARGE_BINARY:
+ case Type::LARGE_STRING:
+ case Type::LARGE_LIST:
+ case Type::LARGE_LIST_VIEW:
+ return type;
+ default:
+ Unreachable();
+ break;
Review Comment:
Unreachable is `[[noreturn]]` so it shouldn't be necessary to complete
control flow after it is called
```suggestion
```
##########
cpp/src/arrow/array/concatenate.cc:
##########
@@ -337,20 +382,40 @@ class ConcatenateImpl {
return ConcatenateBuffers(buffers, pool_).Value(&out_->buffers[1]);
}
- Status Visit(const BinaryType&) {
+ Status Visit(const BinaryType& input_type) {
std::vector<Range> value_ranges;
ARROW_ASSIGN_OR_RAISE(auto index_buffers, Buffers(1, sizeof(int32_t)));
- RETURN_NOT_OK(ConcatenateOffsets<int32_t>(index_buffers, pool_,
&out_->buffers[1],
- &value_ranges));
+ ARROW_ASSIGN_OR_RAISE(
+ auto outcome, ConcatenateOffsets<int32_t>(index_buffers, pool_,
&out_->buffers[1],
+ &value_ranges));
+ switch (outcome) {
+ case OffsetBufferOpOutcome::kOk:
+ break;
+ case OffsetBufferOpOutcome::kOffsetOverflow:
+ switch (input_type.id()) {
+ case Type::BINARY:
+ suggested_cast_ = large_binary();
+ break;
+ case Type::STRING:
+ suggested_cast_ = large_utf8();
+ break;
+ default:
+ DCHECK(false) << "unexpected type id from BinaryType: " <<
input_type;
+ break;
Review Comment:
FWIW Unreachable takes a message
```suggestion
Unreachable("unexpected type id from BinaryType");
```
--
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]
