ARROW-3762: [C++] manage ChunkedArrayBuilder capacity explicitly

[bkietz]

ChunkedArrayBuilder should never request BinaryBuilder reserve space for more strings than it can hold (kListMaximumElements); with this change it will instead of begin a new chunk

[wesm]

This looks okay, but I think we need to add some unit tests (that aren't disabled) to exercise the various code paths in ChunkedBinaryBuilder::Reserve to assert that they do what we expect. Otherwise if we refactor later we're going to be playing whackamole

[pitrou]

Looks like a reasonable improvement to me. Unfortunately it'll be difficult to come up with a test that doesn't blow 16+GB of memory.

[bkietz]

I'll add some tests to src/arrow/array-binary-test.cc

[bkietz]

@pitrou @wesm This new test is pretty minimal but still takes ~4 minutes (or 20s in release) to pass on my machine

[wesm]

Hm. Can we make the 2GB limit something that's configurable rather than hard-coded? In theory that's the purpose of the max chunk size. We can't have such a long-running test in CI

[bkietz]

max_chunk_size_ is a restriction on the character data (string_array.buffer[2]) and not on the number of strings in an array; the test I added appends 2 ** 31 empty strings so chunk_data_size_ never rises from 0. (see also: ArrayBuilder::Reserve which allocates space to store array elements vs BinaryBuilder::ReserveData which allocates space to store character data)

We could certainly add another to apply to the # of strings in the string_array, or alternatively use max_chunk_size_ / sizeof(int32_t) as that limit

[wesm]

Oh okay, sorry for my confusion. Can we make the number of elements per chunk configurable, then? I can see a use case for having no more than 64K elements per chunk, no matter what their size. Then this is much easier to test

[bkietz]

Will do

[wesm]

+1, thanks @bkietz! Will merge once CI has run

[wesm]

Merging, the failure is the cloud.r-project.org DNS issue