ARROW-33: [C++] Implement zero-copy array slicing

[fengguangyuan]

Add two methods to slice an Array. It will generate a new Array
containing a Buffer with parent pointer, offset and length.

[fengguangyuan]

Some confusions follow:
1.If it's necessary to add the slicing methods into ListArray, UnionArray etc.
2.Before slice(...) returns an Array, which is better the Array contains a base class Buffer
or a derived Buffer.
e...Anyway, hope this request not too bad. : )

[tnachen]

assert start >= 0?

[wesm]

Please use the variable naming conventions from https://google.github.io/styleguide/cppguide.html#Variable_Names. Do not use Hungarian notation

[wesm]

Probably use DCHECK_GT for the assertion

[fengguangyuan]

Thanks for your advices, I will modify it according to the expected format.

[fengguangyuan]

Eh....
Actually, this method will accept start and length when they are negative.
Thanks for your advices. I will modify the variable names.
:)

[wesm]

What about other Array types (like ListArray, StringArray)?

[wesm]

Also move this implementation to primitive.cc

[wesm]

One major issue (I think you mentioned on JIRA) is the null bitmap — slicing may require a memory allocation and copying over the bits. Another option is to add a layer of indirection and a Bitmap class that can retrieve bits including an offset, so that zero-copy is possible.

Otherwise, if creating a new bitmap is required, then the API must change to return Status (since the allocation could fail)

[drankye]

Another option is to add a layer of indirection and a Bitmap class that can retrieve bits including an offset, so that zero-copy is possible.

This option sounds better, as similar to the data buffer.

[wesm]

It's a performance trade-off. The downside of that approach is that a bitmap offset then potentially leaks into many algorithms

[fengguangyuan]

@wesm @drankye
Yes, you mentioned features are great.
And I will try to add an indirection layer to do tricks with null bitmap.

Then I still have a confusion with the following code.
auto sliced_buf = std::make_shared<Buffer>(parent_data, int64_t(uFrom * sizeof(T)), int64_t(uLength * sizeof(T)));
For the above code. I am not sure when the slice method returns an specified Array, such as Int32Array, the Array should contain a base-class Buffer object, or a derived-class PoolBuffer object.
After all, Int32Array is created with a PoolBuffer which owns some features that Buffer dosen't.
Maybe, the new code just looks like this:
auto sliced_buf = std::make_shared<PoolBuffer>(parent_data, int64_t(uFrom * sizeof(T)), int64_t(uLength * sizeof(T)));

Anyway, I will rewirte the code based on all of your suggestions.
Thank you, everybody.
Have a nice day. :)

[wesm]

I'm concerned about working more on this patch until we figure out a function evaluation model. See ARROW-34. Zero-copy slicing is difficult unless you construct a view object (definitely an option).

[fengguangyuan]

@wesm
OK. I will reduce the influence of this issue as much as possible.

By the way, it's hard to put the method into Primitive.cc, unless class PrimitiveArray is templatable
or provide more arguments for this method, because user may expect this method ot return a specific
Array, like Int32Array, so some more tricks should be considered to adapt to the change.

Thanks a lot.

[drankye]

To summarize the question Guangyuan mentioned above: we have some Buffer types including Buffer, PoolBuffer and etc., to slice an array, what's the concrete Buffer type to be used by the resultant array? It would be good to use the same Buffer type the parent uses, but not sure how easy it's to be done.

I have a question related. I'm not sure how much benefit we would obtain by having quite a few Buffer subclasses. Might it be just to have one Buffer class, and add all the necessary methods like Append, Resize, Reserve to it? It should simplify something I guess.

[wesm]

Since users of Arrow will have memory originating from many places and with varying ownership semantics, forcing a particular Buffer or memory allocated strategy is not a good idea. By enabling users to implement their own Buffer subclasses and their own memory pools, they can hook the Arrow C++ library more easily into existing applications.

I'm open to other ideas ways to arrange Arrow's internal memory management, reference counting, allocation, and ownership semantics. This probably would need to take the form of a patch or the start of a patch, though.

[fengguangyuan]

@drankye
Yes, maybe your question is good, I am not sure.
@wesm
Great. The user-defined buffer should be nice. Arrow has the rules, so does the users.
Arrow defines the basic measures of internal memory management etc.
An interface of Buffer or memory pool for user definition is needed, I think.

[fengguangyuan]

Why the CI test failed, however it occurs no failures in my environment?
Please. :(

[wesm]

The test suite is failing the valgrind memory leak checks. We enabled this to be printed in parquet (https://github.com/apache/parquet-cpp/blob/master/ci/travis_script_cpp.sh).

I'm pretty concerned about this patch because of the semantics of slicing bitmaps per my April 4 comment. I have not reviewed the code yet.