[C++] BufferReader should zero-copy if and only if given a Buffer, not a pointer and length

[lidavidm]

Describe the enhancement requested

BufferReader can be constructed from a pointer and length:

arrow/cpp/src/arrow/io/memory.cc

Lines 277 to 279 in 0e677d2

	BufferReader::BufferReader(std::string_view data)
	: BufferReader(reinterpret_cast<const uint8_t*>(data.data()),
	static_cast<int64_t>(data.size())) {}

In that case, it will happily hand you a Buffer backed by that pointer:

arrow/cpp/src/arrow/io/memory.cc

Line 353 in 0e677d2

return std::make_shared<Buffer>(data_ + position, nbytes);

However, I don't think the APIs make it clear that this ties the lifetime of any data read from the reader to the lifetime of the input. It would be clearer if it would copy in this case, and require you to explicitly construct BufferReader from a Buffer if you want to enable zero-copy.

Component(s)

C++

[lidavidm]

There's a second issue here: BufferReader has no constructor for std::string, but the implicit conversion from string to string_view means you can still construct a BufferReader that is implicitly going to blow up on you. We should add an explicit constructor to avoid this.

[mapleFU]

So, supports_zero_copy should also decided by if the data is owned?

bool BufferReader::supports_zero_copy() const { return true; }

And can I try to work on this issue?

[lidavidm]

Yeah, I think:

1. Add an explicit constructor for std::string that wraps it in an Arrow buffer, or even prevent construction via string (since it's owned, but not an Arrow buffer)
2. Make zero copy only happen if we are given a buffer, which is a promise about ownership. Otherwise always copy. I think you have to modify ReadAt as well.

Feel free to take this up. Thanks!

[wgtmac]

The ctor of Buffer has a comment implies that the passed memory must be kept alive through some other means.

  /// \brief Construct from buffer and size without copying memory
  ///
  /// \param[in] data a memory buffer
  /// \param[in] size buffer size
  ///
  /// \note The passed memory must be kept alive through some other means
  Buffer(const uint8_t* data, int64_t size)

Does it mean BufferReader should inherit the same implication? The buffer itself cannot tell if it is owned or not after construction. If we have that info on hand, could we handle zero-copy better?

[lidavidm]

There's more discussion on the PR itself, FWIW

[mikelui]

FWIW I think there'd be a lot of value in an "examples" PR that has conventions for doing common patterns (e.g. given I own this buffer/input stream/message stream, how do I pass that ownership to the Arrow RecordBatch/Table I create?)

[mapleFU]

e.g. given I own this buffer/input stream/message stream, how do I pass that ownership to the Arrow RecordBatch/Table I create?

Personally, I think BufferReader should always build from std::string or std::shared_ptr<Buffer>. Other constructor is deprecated, and when using std::shared_ptr<Buffer>, user should gurantee the lifetime of output buffer.

[lidavidm]

@mikelui There's https://arrow.apache.org/cookbook/ / https://github.com/apache/arrow-cookbook ; more examples would always be welcome!