ARROW-13390: [C++] Implement coalesce for remaining types by lidavidm · Pull Request #11080 · apache/arrow

lidavidm · 2021-09-03T19:12:56Z

No description provided.

github-actions · 2021-09-03T19:13:14Z

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

lidavidm · 2021-09-07T18:42:01Z

Converting to draft while I work out the Windows CI issues.

edponce · 2021-09-10T04:45:48Z

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

The loop iterates for Scalars but not for Arrays?

Note the stated purpose of the loop. This is to optimize a special case where we have 0 or more all-null arguments followed by an all-non-null argument.

edponce · 2021-09-10T04:46:34Z

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

Nit: In the first loop, you check for datum.is_array() but not here.

The check above is not just for array. Here there is no need since we know it is either an array or a scalar.

pitrou · 2021-09-16T14:26:12Z

c_glib/test/test-sparse-union-scalar.rb

This is a bit cryptic if you don't know a union is expected. What about e.g. union{number: int8 = -29}?

pitrou · 2021-09-16T14:43:47Z

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

I assume this isn't going to be very performant compared to e.g. raw_builder->Append(source_array.GetView(i)). Not necessarily worth addressing.

Right, we'd have the overhead of a virtual call and some other bookkeeping vs just a direct append call.

pitrou · 2021-09-16T14:47:32Z

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

Do you mean to add a test for this?

Whoops, thanks for the catch. After looking things over I ended up reworking DispatchBest here, adding checks to ensure parameterized types have the same parameters, and adding some basic tests of the helpers we use for promotions.

Actually, one more thing I want to do now is have CheckDispatchBest also ensure that the promoted ValueDescrs from DispatchBest match the ones given to the test.

Done - I also adjusted some tests and added a set of tests specifically for the type promotion helpers we use.

pitrou

Thanks for the update. Just a couple more comments and questions below.

pitrou · 2021-09-21T12:26:46Z

cpp/src/arrow/compute/kernels/codegen_internal.h

Probably a reminder that we'd like a std::span backport at some point ;-)

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

pitrou · 2021-09-21T12:30:53Z

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

Hmm... if saw_date64 is true but saw_date32 false, we should still return date64, right?

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

pitrou · 2021-09-21T12:32:17Z

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

We should still examine other types in case they are incompatible, no?

pitrou · 2021-09-21T12:36:38Z

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

Should there be an error in case BasicDecimal256::kMaxPrecision is exceeded?

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

pitrou · 2021-09-21T12:39:21Z

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

pitrou · 2021-09-21T12:39:45Z

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

Note this makes the name of the function a bit weird ;-)

pitrou · 2021-09-21T12:53:42Z

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

Should we replace "identical" with "compatible"? After all, some implicit casting is allowed.

pitrou · 2021-09-21T13:01:34Z

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

It seems you could simply have:

template <typename Type> struct CoalesceFunctor<Type, std::enable_if<is_nested_type<Type>::value && !is_union_type<Type>::value>::type> { // common implementation for non-union nested types };

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

bkietz · 2021-09-22T18:35:04Z

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

+                        {decimal128(2, 1), decimal128(2, 1)});
      CheckDispatchBest(name, {decimal256(2, 1), decimal256(2, 1)},
-                        {decimal256(3, 1), decimal256(3, 1)});
+                        {decimal256(2, 1), decimal256(2, 1)});


This is surprising to me. Could you comment on why the implicit cast is no longer necessary?

DispatchBest in this case doesn't actually promote either argument. When I adjusted CheckDispatchBest to actually compare the final types against the given types, it exposed discrepancies like this that I fixed. In the logic here, for add/subtract, we do not scale up with the scales are the same:

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

Lines 268 to 273 in 3317f83

switch (promotion) {

case DecimalPromotion::kAdd: {

left_scaleup = std::max(s1, s2) - s1;

right_scaleup = std::max(s1, s2) - s2;

break;

}

Co-authored-by: Benjamin Kietzman <[email protected]>

lidavidm · 2021-09-28T18:31:54Z

Just a ping here for either @pitrou or @bkietz 🙂

pitrou

+1, thank you very much!

Closes apache#11080 from lidavidm/arrow-13390 Authored-by: David Li <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions bot added the Component: C++ label Sep 3, 2021

lidavidm force-pushed the arrow-13390 branch from 03411e6 to 2b34b47 Compare September 7, 2021 18:11

lidavidm marked this pull request as draft September 7, 2021 18:41

github-actions bot added the Component: GLib label Sep 7, 2021

lidavidm marked this pull request as ready for review September 8, 2021 13:25

ianmcook requested a review from bkietz September 8, 2021 14:21

edponce reviewed Sep 10, 2021

View reviewed changes

lidavidm force-pushed the arrow-13390 branch from 85dc802 to 8a71120 Compare September 15, 2021 15:30

pitrou reviewed Sep 16, 2021

View reviewed changes

lidavidm force-pushed the arrow-13390 branch from 8a71120 to 0d6c742 Compare September 16, 2021 20:47

pitrou reviewed Sep 21, 2021

View reviewed changes

lidavidm force-pushed the arrow-13390 branch from f004db9 to 887756f Compare September 21, 2021 13:43

lidavidm added 8 commits September 22, 2021 14:25

ARROW-13390: [C++] Implement ToString for union scalars

882e867

ARROW-13390: [C++] Implement Coalesce for remaining types

46912ee

ARROW-13390: [Ruby] Update scalar to_s tests

f744fca

ARROW-13390: [C++] Try to satisfy MinGW

754e0ce

ARROW-13390: [C++] Address feedback, add dispatch tests

b51c251

ARROW-13390: [C++] Add/fix more dispatch tests

c11c4bd

ARROW-13390: [C++] Fix lint error

56a975c

ARROW-13390: [C++] Address review suggestions

3058559

lidavidm force-pushed the arrow-13390 branch from 887756f to 3058559 Compare September 22, 2021 18:25

bkietz requested changes Sep 22, 2021

View reviewed changes

lidavidm and others added 2 commits September 22, 2021 14:44

Update cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc

90c6a10

Co-authored-by: Benjamin Kietzman <[email protected]>

ARROW-13390: [C++] Add one last test case

3a519e4

lidavidm mentioned this pull request Sep 23, 2021

ARROW-13358: [C++] Improve type support in if_else #11218

Closed

ARROW-13390: [C++] Reconcile with ARROW-13358

63b2fa6

pitrou approved these changes Sep 29, 2021

View reviewed changes

pitrou closed this in b60bbb3 Sep 29, 2021

lidavidm deleted the arrow-13390 branch September 29, 2021 11:57

asfimport mentioned this pull request Sep 30, 2021

[C++] Improve type support for 'coalesce' kernel #29061

Closed

	switch (promotion) {
	case DecimalPromotion::kAdd: {
	left_scaleup = std::max(s1, s2) - s1;
	right_scaleup = std::max(s1, s2) - s2;
	break;
	}

Conversation

lidavidm commented Sep 3, 2021

Uh oh!

github-actions bot commented Sep 3, 2021

Uh oh!

lidavidm commented Sep 7, 2021

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!

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!

pitrou 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!

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!

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!

lidavidm commented Sep 28, 2021

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants