ARROW-549: [C++] Add arrow::Concatenate function to combine multiple arrays into a single Array by bkietz · Pull Request #3746 · apache/arrow

bkietz · 2019-02-25T18:48:48Z

Concatenate arrays into a single array

wesm

This seems to be on the right track. The test coverage is a bit thin, though. It would be better to return NotImplemented in paths that are untested if tests don't get written for them

wesm · 2019-02-25T22:02:57Z

cpp/src/arrow/array-test.cc

How about just writing:

CheckConcatenate(type, arrays_as_json, expected_json)

The ConcatenateParam seems unnecessarily elaborate to me.

It doesn't appear that the non-zero offset cases are being tested right now, so in those cases will have to construct and slice the test arrays manually

wesm · 2019-02-25T22:04:00Z

cpp/src/arrow/array.cc

This implementation needs a more appropriate home than array.cc. We could create a subdirectory of miscellaneous array algorithms. I don't have a strong opinion, @xhochy @pitrou @fsaintjacques @emkornfield thoughts?

under util/ might make sense for now. We can see how many random algorithms get put here vs just used in kernels (my second choice would be under compute I suppose).

Algorithms could be grouped under src/arrow/array/algorithm_<category>.{h,cc} with a single convenience header/source src/arrow/algorithm.{h,cc} similar to builders

If this is all private, we should avoid convenience headers that balloon compile times.

wesm · 2019-02-25T22:05:48Z

cpp/src/arrow/array.cc

Add a comment what this is?

wesm · 2019-02-25T22:09:04Z

cpp/src/arrow/array.cc

I think this should return Invalid if any type is unequal

wesm · 2019-02-25T22:20:48Z

cpp/src/arrow/array.cc

Seems like the null bitmap is getting concatenated twice here

wesm · 2019-02-25T22:26:17Z

I think you need to define 2^31 - 1 overflow behavior for offsets (lists and binary)

bkietz · 2019-02-27T18:00:15Z

@fsaintjacques re property testing

fsaintjacques · 2019-02-28T14:36:27Z

cpp/src/arrow/buffer.h

Do we want this at the top-level namespace, maybe static Status Buffer::Concatenate(...) or BuffersConcatenate()?

the renaming seems unnecessary- the overload will not conflict with concatenate(arrays)

I'm not concerned about conflict. I'm concerned about readability. buffer::Concatenate or Buffer::Concatenate speaks a lot more than just Concatenate.

IMHO it's more readable to avoid manual name mangling; the argument types are part of what users will read

Would it help to introduce an alias like:

using BufferVector = std::vector<std::shared_ptr<Buffer>>

I think this would help make the signature easier to read.

cpp/src/arrow/util/concatenate.cc

fsaintjacques · 2019-02-28T18:11:23Z

cpp/src/arrow/util/concatenate.cc

Ditto, there's no need for this function to depends on internal state. In fact, I'd want to see this function explicitly unit tested.

cpp/src/arrow/util/concatenate.cc

bkietz · 2019-03-05T20:10:32Z

@wesm @fsaintjacques how's this look?

hatemhelal · 2019-03-07T20:24:01Z

cpp/src/arrow/util/concatenate.h

I think you could use the ArrayVector alias in the first input argument.

Great, that will neaten this up quite a bit. I see there's also BufferVector which should be useful too

hatemhelal · 2019-03-08T13:26:01Z

cpp/src/arrow/array-test.cc

The Mersenne-Twister PRNG engine has been identified as a source of test slowness. See discussion here.

Alright, I'll switch to std::default_random_engine

hatemhelal · 2019-03-08T13:27:33Z

cpp/src/arrow/array-test.cc

Another place where the ArrayVector alias could be used.

cpp/src/arrow/array-test.cc

pitrou

Thanks for doing this. Here are some comments below. In particular, I would welcome a bit more clarity in the implementation.

cpp/src/arrow/array-test.cc

pitrou · 2019-03-18T18:31:32Z

cpp/src/arrow/array-test.cc

Is it? What if I have sliced a StringArray?

It's in the spec: the first offset in an offsets buffer is always 0.

pitrou · 2019-03-18T18:31:58Z

cpp/src/arrow/array-test.cc

Same question here.

pitrou · 2019-03-18T18:32:26Z

cpp/src/arrow/array-test.cc

I would rather have this in a separate test file. array-test.cc is already too large, and too slow to compile.

Alright, I'll move it to arrow-concatenate-test.

cpp/src/arrow/util/concatenate.cc

pitrou · 2019-03-18T18:51:23Z

cpp/src/arrow/buffer.h

Please call this ConcatenateBuffers.

Should I rename Concatenate(arrays) to ConcatenateArrays as well?

Yes, you can, though disambiguating requires only one rename ;-)

pitrou · 2019-03-18T18:53:51Z

cpp/src/arrow/util/concatenate.cc

Can you add a comment explaining what this does?

pitrou · 2019-03-18T18:54:02Z

cpp/src/arrow/util/concatenate.cc

Can you add a comment explaining what this does?

cpp/src/arrow/util/concatenate.cc

- test concatenation of randomly generated array slices - ensure some of those array slices will have offset != 0 - test all primitive types, StringType, ListType, StructType, and DictionaryType - add disabled test for UnionType

vagrind was complaining about uninitialized values due to the way CopyBitmap restores trailing bits; extended CopyBitmap to (optionally) elide that step

- renaming - use memcpy not std::copy in ConcatenateBuffers - concatenating zero buffers does not terminate - refactor for clarity - add comments

bkietz · 2019-03-20T17:45:23Z

@pitrou does this address your concerns?

pitrou · 2019-03-20T17:52:30Z

Thanks @bkietz. Can you open a JIRA for concatenation of union arrays?

bkietz · 2019-03-20T17:57:05Z

https://issues.apache.org/jira/browse/ARROW-4975

wesm reviewed Feb 25, 2019

View reviewed changes

wesm changed the title ~~ARROW-549: [C++] concatenate function~~ ARROW-549: [C++] Add arrow::Concatenate function to combine multiple arrays into a single Array Feb 25, 2019

bkietz force-pushed the ARROW-549-concatenate-arrays branch from 573be68 to 4ef76e7 Compare February 27, 2019 17:53

fsaintjacques requested changes Feb 28, 2019

View reviewed changes

hatemhelal reviewed Mar 7, 2019

View reviewed changes

hatemhelal reviewed Mar 8, 2019

View reviewed changes

cpp/src/arrow/array-test.cc Outdated

Copy link

Contributor

hatemhelal Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another place where the ArrayVector alias could be used.

hatemhelal reviewed Mar 8, 2019

View reviewed changes

cpp/src/arrow/array-test.cc Outdated Show resolved Hide resolved

pitrou requested changes Mar 18, 2019

View reviewed changes

bkietz added 18 commits March 20, 2019 11:00

fist pass at concatenate function

efc34f3

move Concatenate to util/concatenate

2573aca

don't concatenate null bitmaps twice

0c524f3

return Invalid on mismatched types

cbb89c5

bail on offset overflow

a953826

remove unnecessary shared_ptr indirection

711de4a

Improve Concatenate testing

acc6c08

- test concatenation of randomly generated array slices - ensure some of those array slices will have offset != 0 - test all primitive types, StringType, ListType, StructType, and DictionaryType - add disabled test for UnionType

remove ConcatenateParam, fix concat(Dictionary)

5803f1d

add lint #includes

d8ba14d

assert trailing bits are zeroed in bitmap buffers

76ac101

vagrind was complaining about uninitialized values due to the way CopyBitmap restores trailing bits; extended CopyBitmap to (optionally) elide that step

remove offsets_, lengths_, range->Range

94460a8

get out_ correctly shaped in constructor

aa3d399

use less_than rather than not_equal for loop conditions

d58d063

Refactor SliceData to ArrayData::Slice

6d8b76f

refactor ConcatenateImpl::Visit to be thinner with helpers

c6686a4

refactor bitmap concatenation

25a040e

fix implicit conversion warning

7fcd9c7

use ArrayVector, default_random_engine

beb3ad2

bkietz added 3 commits March 20, 2019 11:00

move BufferVector alias to buffer.h and use for Concatenate

aeb2626

Refactor with Antoine's recommendations

ffa58ec

- renaming - use memcpy not std::copy in ConcatenateBuffers - concatenating zero buffers does not terminate - refactor for clarity - add comments

move concatenate test to separate file

c1600bc

bkietz force-pushed the ARROW-549-concatenate-arrays branch from 14a78f7 to c1600bc Compare March 20, 2019 17:41

pitrou approved these changes Mar 20, 2019

View reviewed changes

pitrou closed this in 43f2a31 Mar 20, 2019

bkietz deleted the ARROW-549-concatenate-arrays branch March 20, 2019 19:56

This was referenced Mar 20, 2019

[C++] Add function to concatenate like-typed arrays #16186

Closed

[C++] Support concatenation of UnionArrays #16035

Closed

Comments

Conversation

bkietz commented Feb 25, 2019

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Feb 25, 2019

Uh oh!

bkietz commented Feb 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz commented Mar 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!