ARROW-13222: [C++] Improve type support for case_when #10806

lidavidm · 2021-07-26T20:17:31Z

This adds support for nearly everything except dictionaries and extension types. Binary types, maps, lists (fixed-size and variable), unions, and structs are supported. The benchmark is also extended to show that performance is not entirely terrible (the first attempt using purely AppendScalar ran at ~50MB/s, this runs at a couple GB/s similar to the other kernel cases). This does leverage AppendScalar for the scalar cases.

Dictionaries are still supported, but will be unpacked. For direct support there's work described in ARROW-13573, but I'd like to validate my approach here before making the changes necessary.

github-actions · 2021-07-26T20:17:55Z

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

lidavidm · 2021-08-04T14:57:39Z

Moved back to draft, need to implement full dictionary support as described in #10724 (comment)

lidavidm · 2021-08-05T14:32:24Z

Moved out of draft - dictionary support will need more work and I want to validate the approach here first. Filed ARROW-13573 to cover that work.

pitrou

Very nice, especially the thorough testing. Here are a couple comments.

cpp/src/arrow/array/util.cc

cpp/src/arrow/buffer_builder.h

cpp/src/arrow/compute/kernels/scalar_if_else_benchmark.cc

cpp/src/arrow/compute/kernels/scalar_if_else.cc

pitrou · 2021-08-18T16:36:11Z

cpp/src/arrow/compute/kernels/scalar_if_else.cc

@@ -1413,6 +1417,588 @@ struct CaseWhenFunctor<NullType> {
  }
 };

+static Status ExecVarWidthScalarCaseWhen(KernelContext* ctx, const ExecBatch& batch,
+                                         Datum* out) {


Why not return Result<datum>?

It's minor but 1) we use the shape of out to determine what to return below and 2) it fits slightly better with the rest of the APIs as they all use the Status(..., Datum* out) pattern.

cpp/src/arrow/compute/kernels/scalar_if_else.cc

pitrou · 2021-08-18T16:52:23Z

cpp/src/arrow/compute/kernels/scalar_if_else.cc

+
+static Status GetValueAppenders(const DataType& type, ArrayAppenderFunc* array_appender);
+
+struct GetAppenders {


Hmm... this functionality may be useful elsewhere (and actually, perhaps it can already be reused in other places).

Why not add a virtual method to ArrayBuilder (e.g. virtual AppendArraySlice(...)) instead of keeping this private?

I was debating this. I'll refactor this to be part of the builder instead.

pitrou

Some more comments. Thanks a lot for the update!

pitrou · 2021-08-19T13:54:24Z

cpp/src/arrow/array/builder_base.h

+                               " but got array of type ", *array.type);
+    }
+    return AppendArraySliceUnchecked(array, offset, length);
+  }


Hmm, I would not bother with the checked version personally. Just mention in the docstring that the types are supposed to be identical.

pitrou · 2021-08-19T13:55:06Z

cpp/src/arrow/array/builder_base.h

+                                           const int64_t length) {
+    return Status::NotImplemented("AppendArraySliceUnchecked for builder for ", *type());
+  }
+  // TODO: overloads for arrays


Either do it just here, or just remove the TODO, IMHO. It's not worth keeping this comment for a trivial two-liner.

cpp/src/arrow/array/builder_binary.h

pitrou · 2021-08-19T14:01:16Z

cpp/src/arrow/array/builder_nested.h

+    }
+    const uint8_t* validity = array.MayHaveNulls() ? array.buffers[0]->data() : nullptr;
+    for (int64_t row = offset; row < offset + length; row++) {
+      RETURN_NOT_OK(Append(!validity || BitUtil::GetBit(validity, array.offset + row)));


Can't we just do a bulk-append of the validity bitmap here?

pitrou · 2021-08-19T14:02:55Z

cpp/src/arrow/array/builder_union.h

@@ -155,6 +156,21 @@ class ARROW_EXPORT DenseUnionBuilder : public BasicUnionBuilder {
    return offsets_builder_.Append(offset);
  }

+  Status AppendArraySliceUnchecked(const ArrayData& array, const int64_t offset,
+                                   const int64_t length) override {


The definition can probably moved to builder_union.cc instead?
(same for SparseUnionBuilder)

pitrou · 2021-08-19T14:04:45Z

cpp/src/arrow/array/util.cc

    out_->buffers[1] = buffer_;
+    // buffer_ is zeroed, but 0 may not be a valid type code
+    if (type.type_codes()[0] != 0) {
+      ARROW_ASSIGN_OR_RAISE(out_->buffers[1], AllocateBuffer(buffer_->size(), pool_));


length_ rather than buffer_->size()? (can buffer_ be larger because of the children or offsets?)

pitrou · 2021-08-19T14:04:54Z

cpp/src/arrow/array/util.cc

+    if (type.type_codes()[0] != 0) {
+      ARROW_ASSIGN_OR_RAISE(out_->buffers[1], AllocateBuffer(buffer_->size(), pool_));
+      std::memset(out_->buffers[1]->mutable_data(), type.type_codes()[0],
+                  buffer_->size());


length_ here too?

cpp/src/arrow/compute/kernels/scalar_if_else.cc

pitrou · 2021-08-19T14:09:58Z

cpp/src/arrow/compute/kernels/scalar_if_else_test.cc

+  auto values1 = ArrayFromJSON(type, R"(["cDE", null, "degfhi", "efg"])");
+  auto values2 = ArrayFromJSON(type, R"(["fghijk", "ghi", null, "hi"])");
+
+  // CheckScalar("case_when", {MakeStruct({}), values1}, values1);


Did you forget to uncomment all these?

Sorry, I pushed something half WIP, I'll comment again once things are ready. Thanks for taking a look.

… types

…ceUnchecked" This reverts commit fe9ecbf0a35117db7ad955ef345181328633277b.

pitrou · 2021-08-19T17:16:36Z

cpp/src/arrow/array/builder_base.h

+  /// \brief Append a range of values from an array.
+  ///
+  /// The given array must be the same type as the builder.
+  virtual Status AppendArraySliceUnchecked(const ArrayData& array, int64_t offset,


We can drop "Unchecked" from the name, I think, as long as the contract is documented.

lidavidm · 2021-08-19T19:18:44Z

Hmm, there's a Windows R/MinGW-specific failure :/

lidavidm · 2021-08-19T23:31:40Z

Alright, well it's crashing on Windows because the std::function introduced into ExecVarWidthArrayCaseWhen contains not the lambda, but some completely random function which stomps all over the stack. That's about as far as I'm going to get today (skipping the call to reserve_data lets the test pass just fine).

… toolchain

pitrou

+1. Can you also update the documented input types in the Sphinx docs?

lidavidm · 2021-08-23T15:56:03Z

Thanks. I updated the docs, I left a note about dictionary types that will be updated in ARROW-13573.

github-actions bot added the Component: C++ label Jul 26, 2021

lidavidm force-pushed the arrow-13222 branch from e4f1605 to df0d4fa Compare August 3, 2021 12:57

lidavidm marked this pull request as draft August 3, 2021 15:33

lidavidm force-pushed the arrow-13222 branch from df0d4fa to 2e9777c Compare August 3, 2021 18:28

lidavidm marked this pull request as ready for review August 3, 2021 19:17

lidavidm marked this pull request as draft August 4, 2021 14:57

lidavidm marked this pull request as ready for review August 5, 2021 14:24

lidavidm force-pushed the arrow-13222 branch from 2e9777c to 98004be Compare August 5, 2021 14:32

lidavidm force-pushed the arrow-13222 branch from 98004be to 7d79ac4 Compare August 10, 2021 14:06

ianmcook requested a review from pitrou August 17, 2021 20:39

pitrou reviewed Aug 18, 2021

View reviewed changes

lidavidm force-pushed the arrow-13222 branch from 7d79ac4 to aea14a4 Compare August 18, 2021 21:13

pitrou reviewed Aug 19, 2021

View reviewed changes

lidavidm added 14 commits August 19, 2021 13:13

ARROW-13222: [C++] Improve type support for case_when kernel

8a5545f

ARROW-13222: [C++] Add list type support for case_when kernel

2202b74

ARROW-13222: [C++] Add more benchmarks for case_when kernel

737347a

ARROW-13222: [C++] Add basic implementations for map, fixed size list

6ffb272

ARROW-13222: [C++] Much faster, but less generic case_when for nested…

0d42d40

… types

ARROW-13222: [C++] Expand type support

3ec727a

ARROW-13222: [C++] Expand type support further

d6b491c

ARROW-13222: [C++] Fix typos

a7a3b45

ARROW-13222: [C++] Fix types

d6a50ea

ARROW-13222: [C++] casts for msyc

1f47c6d

ARROW-13222: [C++] Explicit cast to avoid accidental overflow

a4336f8

ARROW-13222: [C++] Add support for unions

2bde163

ARROW-13222: [C++] Trim unnecessary cases

5307c5a

ARROW-13222: [C++] Refactor GetValueAppender into builder methods

72811bb

lidavidm added 9 commits August 19, 2021 13:13

ARROW-13222: [C++] Untemplate common case

d9cb23e

ARROW-13222: [C++] Address feedback

c834b5c

ARROW-13222: [C++] Add tests for AppendArraySliceUnchecked

2b6dd26

ARROW-13222: [C++] Fix lint/MSVC warnings

7390452

ARROW-13222: [C++] Optimize BaseBinaryBuilder::AppendArraySliceUnchecked

bd95fc8

ARROW-13222: [C++] Optimize StructBuilder::AppendArraySliceUnchecked

bf01084

ARROW-13222: [C++] Reorganize union builders

b3c12a1

ARROW-13222: [C++] Don't over-allocate

0324fd1

Revert "ARROW-13222: [C++] Optimize BaseBinaryBuilder::AppendArraySli…

0ac03c1

…ceUnchecked" This reverts commit fe9ecbf0a35117db7ad955ef345181328633277b.

lidavidm force-pushed the arrow-13222 branch from aea14a4 to 0ac03c1 Compare August 19, 2021 17:13

pitrou reviewed Aug 19, 2021

View reviewed changes

lidavidm added 4 commits August 19, 2021 13:21

ARROW-13222: [C++] Rename to AppendArraySlice

e5581e6

ARROW-13222: [C++] Add forgotten RETURN_NOT_OK

ee1a6b0

ARROW-13222: [C++] Add another forgotten RETURN_NOT_OK

ef62b84

ARROW-13222: [C++] Fix qualifiers for MSVC2015

b52dab8

ARROW-13222: [C++] Add cast for MSVC

814b2f3

lidavidm added 2 commits August 20, 2021 09:47

ARROW-13222: [C++] Try to avoid apparently miscompilation under MinGW…

2f67f7f

… toolchain

ARROW-13222: [C++] Try to fix MinGW again

4cc6387

pitrou approved these changes Aug 23, 2021

View reviewed changes

ARROW-13222: [C++] Update compute.rst

f032791

pitrou closed this in 9e38494 Aug 23, 2021

asfimport mentioned this pull request Sep 23, 2021

[C++] Support variable-width types in case_when function #28909

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-13222: [C++] Improve type support for case_when #10806

ARROW-13222: [C++] Improve type support for case_when #10806

lidavidm commented Jul 26, 2021 •

edited

Loading

github-actions bot commented Jul 26, 2021

lidavidm commented Aug 4, 2021

lidavidm commented Aug 5, 2021

pitrou left a comment

pitrou Aug 18, 2021

lidavidm Aug 18, 2021

pitrou Aug 18, 2021

lidavidm Aug 18, 2021

pitrou left a comment

pitrou Aug 19, 2021

pitrou Aug 19, 2021

pitrou Aug 19, 2021

pitrou Aug 19, 2021

pitrou Aug 19, 2021

pitrou Aug 19, 2021

pitrou Aug 19, 2021

lidavidm Aug 19, 2021

pitrou Aug 19, 2021

lidavidm commented Aug 19, 2021

lidavidm commented Aug 19, 2021

pitrou left a comment

lidavidm commented Aug 23, 2021


		static Status GetValueAppenders(const DataType& type, ArrayAppenderFunc* array_appender);

		struct GetAppenders {

ARROW-13222: [C++] Improve type support for case_when #10806

ARROW-13222: [C++] Improve type support for case_when #10806

Conversation

lidavidm commented Jul 26, 2021 • edited Loading

github-actions bot commented Jul 26, 2021

lidavidm commented Aug 4, 2021

lidavidm commented Aug 5, 2021

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Aug 19, 2021

lidavidm commented Aug 19, 2021

pitrou left a comment

Choose a reason for hiding this comment

lidavidm commented Aug 23, 2021

lidavidm commented Jul 26, 2021 •

edited

Loading