Skip to content

ARROW-13130: [C++] Add decimal support to arithmetic kernels#11313

Closed
lidavidm wants to merge 2 commits into
apache:masterfrom
lidavidm:arrow-13130
Closed

ARROW-13130: [C++] Add decimal support to arithmetic kernels#11313
lidavidm wants to merge 2 commits into
apache:masterfrom
lidavidm:arrow-13130

Conversation

@lidavidm

@lidavidm lidavidm commented Oct 4, 2021

Copy link
Copy Markdown
Member

This adds decimal support for the following kernels (and _checked variants where applicable): abs, acos, add/sub/mul/div, asin, atan, atan2, ceil, cos, floor, hash_stddev, hash_tdigest, hash_variance, is_finite, is_inf, is_nan, ln, log1p, log2, logb, mode, negate, power, quantile, round, round_to_multiple, sign, sin, stddev/variance, tan, tdigest, trunc

Most kernels cast decimals to double and proceed. Some, including rounding, directly operate on decimals. Aggregate kernels directly operate on decimals (and cast to double inline) since DispatchBest is not usable for the aggregate nodes (at least, unless we also reimplement implicit casting there).

Additionally, ValidateFull for scalars/arrays now checks FitsInPrecision. A number of tests were adjusted to account for this.

@github-actions

github-actions Bot commented Oct 4, 2021

Copy link
Copy Markdown

@lidavidm

lidavidm commented Oct 4, 2021

Copy link
Copy Markdown
Member Author

Apologies in advance to whoever tries to review this. If it helps, I can try to split along these lines:

  • Non-kernel changes (changes to ValidateFull and fixing related tests),
  • Arithmetic kernels except rounding,
  • Rounding kernels

@westonpace

Copy link
Copy Markdown
Member

This can be a follow-up but can we modify compute.rst to flag which operations cast decimal to double? Probably as a new flag.

@lidavidm

lidavidm commented Oct 5, 2021

Copy link
Copy Markdown
Member Author

This can be a follow-up but can we modify compute.rst to flag which operations cast decimal to double? Probably as a new flag.

D'oh, I knew I was forgetting something - I'll update the docs. I'll also split out the array validation changes and the rounding kernel here (since that kernel is much more involved and honestly could probably use some cleanup).

Comment thread cpp/src/arrow/compute/kernels/aggregate_tdigest.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/aggregate_tdigest.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/aggregate_var_std.cc Outdated

@cyb70289 cyb70289 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @lidavidm, this is a lot of work to add decimal support to all these kernels.
I didn't review hash aggregate related code, all other changes LGTM.

@lidavidm

lidavidm commented Nov 1, 2021

Copy link
Copy Markdown
Member Author

@pitrou would you be able to take a look at the hash aggregate changes here?

Comment thread cpp/src/arrow/compute/kernels/aggregate_mode.cc Outdated
Comment thread cpp/src/arrow/compute/kernels/aggregate_test.cc Outdated
Comment thread docs/source/cpp/compute.rst Outdated

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, thank you

@pitrou pitrou closed this in 6f4c991 Nov 4, 2021
@ursabot

ursabot commented Nov 4, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 3626a08 and contender = 6f4c991. 6f4c991 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.49% ⬆️0.54%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants