[C++][Statistics] Fix the ownership handling of arrow::ArrayData::statistics in arrow::ArrayData::CopyTo and arrow::ArrayDataViewOrCopy

[andishgar]

Describe the bug, including details regarding any error messages, version, and platform.

As discussed here, the following line should either share or copy arrow::ArrayData::statistics, depending on whether the buffers are copied or viewed.

arrow/cpp/src/arrow/array/data.cc

Lines 167 to 169 in 8afaf95

	output->statistics = data.statistics;

Component(s)

C++

[andishgar]

@kou
I would like to start by implementing an initial solution. Then, we can discuss in the relevant pull request the range of better or more correct approaches that may exist.

However, I have one question: if some buffers are viewed and others are copied in the array, should arrow::ArrayData::statistics be copied or shared?

[kou]

When does the mixed case happen?

[andishgar]

It's possible to create an arrow::Array from multiple arrow::Buffer instances via arrow::ArrayData, and each arrow::Buffer can have its own memory manager, and device allocation type. the behavior of
arrow::ArrayData::ViewOrCopyTo depends on the behavior of MemoryManager fo to parameter in arrow::ArrayData::ViewOrCopyTo and MemoryManager and DeviceAllocationType type of a buffer according to below codes

arrow/cpp/src/arrow/device.cc

Lines 103 to 117 in cfff4d5

	Result<std::shared_ptr<Buffer>> MemoryManager::ViewBuffer(
	const std::shared_ptr<Buffer>& buf, const std::shared_ptr<MemoryManager>& to) {
	if (buf->memory_manager() == to) {
	return buf;
	}
	const auto& from = buf->memory_manager();
	auto maybe_buffer = to->ViewBufferFrom(buf, from);
	COPY_BUFFER_RETURN(maybe_buffer, to);
	// `to` doesn't support viewing from `from`, try the other way
	maybe_buffer = from->ViewBufferTo(buf, to);
	COPY_BUFFER_RETURN(maybe_buffer, to);
	return Status::NotImplemented("Viewing buffer from ", from->device()->ToString(),
	" on ", to->device()->ToString(), " not supported");
	}

arrow/cpp/src/arrow/device.cc

Lines 221 to 234 in cfff4d5

	Result<std::shared_ptr<Buffer>> CPUMemoryManager::ViewBufferFrom(
	const std::shared_ptr<Buffer>& buf, const std::shared_ptr<MemoryManager>& from) {
	if (!from->is_cpu()) {
	return nullptr;
	}
	// in this case the memory manager we're coming from is visible on the CPU,
	// but uses an allocation type other than CPU. Since we know the data is visible
	// to the CPU a "View" of this should use the CPUMemoryManager as the listed memory
	// manager.
	if (buf->device_type() != DeviceAllocationType::kCPU) {
	return std::make_shared<Buffer>(buf->address(), buf->size(), shared_from_this(), buf);
	}
	return buf;
	}

.

1. Are there any practical examples where an arrow::Array contains buffers from different memory managers and device allocation type, or is this scenario purely theoretical?

[andishgar]

@kou, the example below demonstrates a practical case where one buffer is viewed and the other is copied (when cpu_memory_manager_ is passed). I will send a pull request soon to clarify this further

TEST_F(TestArrayDataStatisticsViewOrCopyTO, CudaBufferAndCudaBufferPointToCpuMemory) {
  ASSERT_OK_AND_ASSIGN(auto bitmap_buffer, GenerateGPUBitmapBufferPointToCpuMemory(10));
  ASSERT_OK_AND_ASSIGN(auto data_buffer, GenerateGPUtDataBuffer(
                                             {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}));
  auto array_data = ArrayData::Make(int32(), 10, {bitmap_buffer, data_buffer});
  auto statistics = std::make_shared<ArrayStatistics>();
  statistics->max = 10;
  array_data->statistics = statistics;
  array_data->null_count = 4;
  // Why does the assertion below fail?
  // ASSERT_TRUE(is_valid_as_array(array_data));
  {
    // Since it's not possible to get  a host pointer from data_buffer,
    //data_buffer is copied. (bitmap_buffer is viewed)
    ASSERT_OK_AND_ASSIGN(auto copied_array_data,
                         array_data->ViewOrCopyTo(cpu_memory_manager_));
    ASSERT_FALSE(is_statistics_viewed(array_data, copied_array_data));
  }
  {
    // Despite using the same memory manager, data_buffer is still being copied.
    // Should we consider this a bug?
    ASSERT_OK_AND_ASSIGN(auto copied_array_data,
                         array_data->ViewOrCopyTo(cuda_memory_manager_));
    ASSERT_FALSE(is_statistics_viewed(array_data, copied_array_data));
  }
  {
    // Since neither the buffer nor the 'to' memory manager belong to the CPU,
    // the buffers are copied.
    ASSERT_OK_AND_ASSIGN(auto copied_array_data,
                         array_data->ViewOrCopyTo(my_cuda_memory_manager));
    ASSERT_FALSE(is_statistics_viewed(array_data, copied_array_data));
  }
}