C++: Struct type builder API

[asfimport]

At the moment, we are able to represent struct metadata but not actually build struct-type arrays.

Reporter: Wes McKinney / @wesm
Assignee: Feng Guangyuan / @fengguangyuan

_{Note: This issue was originally created as ARROW-60. Please see the migration documentation for further details.}

[asfimport]

Kai Zheng / @drankye:
Let me understand the type and implement it first.

[asfimport]

Kai Zheng / @drankye:
There were some discussions in this PR by the way. For convenient copying here:

Question from Kai:

I'm trying to think about how to implement this struct array type, by the way, seems to need a container for all the child arrays, like vector, to put all the fields in the declaring order, right? Another question is how to get/set a field? By name, but no name in the layout, so only possible by index I guess. Raise my questions here by the way, thx.
Replying from [~emkornfield@gmail.com]:
Its probably better to discuss struct implementation on the relevant JIRA item for it, but it following the existing pattern Builder with a separate final immutable data structure pattern. Using a std::vector could work for this, just be careful not to do anything in the builder's Finish that could fail. Also, I think indices for getters are the right level of abstraction for this layer (a different layer can map name->index). I'm currently working through some use cases with the ListBuilder and that has made me realize the current builder APIs are super user friendly.

[asfimport]

Micah Kornfield / @emkornfield:
my comment should read, they "are not super user friendly". Or at least a little hard for someone new the code (e.g. me) to understand. The struct builder might be a little challenging to do in C++ from an API perspective, I need to think about it a little bit more and think through the use-cases for it (one use case it being able to convert a tuple from python to a struct: examples for other types are here https://github.com/apache/arrow/blob/master/python/src/pyarrow/adapters/builtin.cc). It might pay to do the implementation for StructArray first and then come back to the builder API once the StructArray implementation looks reasonable.

[asfimport]

Kai Zheng / @drankye:
Thanks Micah for the clarifying, and suggestion. Very helpful!

[asfimport]

Micah Kornfield / @emkornfield:
Hi Kai,
I had a chance to think this through and wanted to know what you thought about the following: I think the implementation of the builder should actually be pretty straightforward. You should be able to extend ArrayBuilder (https://github.com/apache/arrow/blob/master/cpp/src/arrow/builder.h).
You can likely have two constructors:

1. One that takes a list of builders and generated the type for the ArrayBuilder class on the fly
2. One that takes the type (a struct) and generates the builders on the fly (https://github.com/apache/arrow/blob/master/cpp/src/arrow/types/construct.h would be useful for this. I think it might pay to add a version of the method that takes a unique_ptr as an output parameter to make it easier for this use-case.)

For additional methods: Add an Append method modeled after (https://github.com/apache/arrow/blob/master/cpp/src/arrow/types/list.h#L144). It would be be slightly different since you don't need to keep track of offsets. It would be nice if appending a null value also appended a null to each of the child builders, but that would require a new virtual method "AppendNull" on ArrayBuilder. I think its worth it as it would make client code a little bit more concise, but I think there might be some debate due to performance and API consistency.

I don't think a "vectorized" append is needed yet, unless you can think of a use-case for one.

Have you made any progress? How does this align with your thinking?

[asfimport]

Kai Zheng / @drankye:
Thanks Micah for the complete thoughts! I thought it's doable for me. I will complete it this week and hope to provide a PR for your review not too late. Does this plan work for you? If necessary I surely can be earlier. Thanks.

[asfimport]

Micah Kornfield / @emkornfield:
I'm currently not blocked on it. I'm also not a committer, so Wes will ultimately have to do the final merge. But I will be happy to take a first pass at review whenever you have it available.

[asfimport]

Kai Zheng / @drankye:
Thanks Micah for the sync and review in advance.

[asfimport]

Micah Kornfield / @emkornfield:
looking more at the code I think the virtual method call will definitely be a concern, probably best not do it for the time being.

[asfimport]

Kai Zheng / @drankye:
Which virtual method call did you mention? I thought for these builders it maybe ok to have virtual calls unless it's propagational to the real data passed to.

I'm reading the codes as you suggested above. It looks not clear for me class ListBuilder public extends Int32Builder. Not sure there is a relationship here but I guess it's to share and reuse some variables and methods?

[asfimport]

Micah Kornfield / @emkornfield:
A virtual method for AppendNull, but it looks like we are consciously avoiding anything in a tight loop using virtual methods.

Yes, ListBuilder was making use of the fact that the new data a list needs to allocate is bitmap + a buffer of int32s but it is kind of hacky. I am working on trying to refactor it to something that makes more logical sense to me as part of the List IPC stuff.

[asfimport]

Kai Zheng / @drankye:
Ok sound good Micah. I will try to refrain from refactoring the codes here to avoid the conflict. I suggest you open issue separately for the refactoring things.

[asfimport]

Kai Zheng / @drankye:
Sorry I'm distracted and not able to work on this. I had an off-line discussion with [~qq272101359@163.com] this morning and he is working on the issue. He will be dedicated and get this done this week. Hope this works. Thanks!

[asfimport]

Feng Guangyuan / @fengguangyuan:
A draft of implementation for this issue is refered to PR:
#66

[asfimport]

Wes McKinney / @wesm:
Issue resolved by pull request 66
#66