ARROW-2102: [C++] Implement Take kernel

[bkietz]

This implements take as a BinaryKernel

Out of bounds indices raise an error. All integer index types should be supported.

Supported value types are numeric, boolean, null, binary, dictionary, and string (untested: fixed width binary, time/date).

In addition to TakeKernel, a convenience function is implemented which takes arrays as its arguments (currently only array inputs are supported).

[pcmoritz]

This looks great! The take.h header should also be added to compute/api.h.

[fsaintjacques]

Why is it a reference, can it be the actual ptr?

[bkietz]

Mostly I was following the convention that output arguments are passed by pointer.

Since I only make one instance of TakeParameters and pass it around by const& its data members are not mutable. I can refactor if you think it's better to:

• mark out mutable
• make copies of TakeParameter
• pass TakeParameter around by mutable reference

[pitrou]

@bkietz , could you rebase please?

[bkietz]

@pitrou done

[pitrou]

Since the inputs are user-provided, I think the only behaviour that makes sense here is RAISE. TONULL doesn't sound useful at all, while UNSAFE is downright dangerous. @wesm

[bkietz]

UNSAFE is dangerous, but because it is more performant it seems like something users will want in situations where they know bounds checking is definitely unnecessary.

I confess that TONULL was speculative generality.

@wesm is offline for a few weeks, @kou @xhochy ?

[pitrou]

Here are some comments. I would like to see some tests with at least one non-int8 index type (for example uint32).

[kszucs]

While I think RAISE, UNSAFE and even TO_NULL make sense to have, these affect Take's behavior so fundamentally, that We should have different APIs for them instead of passing as an option.

AFAICT the rest of the compute functions follow the RAISE behavior, so I think Take should do as well.

To support UNSAFE / more performant alternatives I suggest to choose a convention We can apply across the whole compute module, for example having UnsafeTake (similarly like other functions are named in arrow) rather than passing it as an option.

Lastly We could express TO_NULL differently, like using Take in conjunction with another kernel function which fills the arrays with nulls.

[bkietz]

Okay, done. Thanks!

[kszucs]

Could You please create issues about adding an Unsafe<Kernel> API and handling the TO_NULL case? Then We can further discuss them separately.

@pitrou what do You think?

[pitrou]

I think the TO_NULL case doesn't make sense, so you can just create a JIRA about potential unsafe variants.

[pitrou]

The CI failures are unrelated (see ARROW-5148).