GH-35273: [C++] Add integer round kernels #36289

js8544 · 2023-06-25T11:03:04Z

Rationale for this change

Currently round casts integers to floats which causes undesired behavior.

What changes are included in this PR?

Add round kernels for integer types.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Closes: [C++] Support "round" kernel for integer inputs #35273

westonpace · 2023-06-29T13:58:36Z

CC @EpsilonPrime do you think you could take a look?

EpsilonPrime · 2023-06-30T08:22:39Z

Hi, I've been looking through the PR and it looks pretty good so far. I'm going to take another pass to see if any of this makes it harder to implement round for Decimal types (I suspect not but it's worth checking). In the meantime could you add benchmarks for the new kernels? Thanks!

EpsilonPrime · 2023-06-30T08:25:42Z

Actually it looks like the benchmarks are already there. I'll run them locally to see what kind of improvement there is. I suspect there will be a noticeable improvement from this PR because the expensive conversion to float won't need to occur.

js8544 · 2023-06-30T16:20:48Z

Actually it looks like the benchmarks are already there. I'll run them locally to see what kind of improvement there is. I suspect there will be a noticeable improvement from this PR because the expensive conversion to float won't need to occur.

https://gist.github.com/js8544/8471c3106bbaff473fb7bddf4c56b4de I just ran it locally and here's the result.

EpsilonPrime · 2023-06-30T22:53:59Z

docs/source/cpp/compute.rst

-  rounding to the nearest multiple of 100 (zeroing the ones and tens digits).
-  Default value of ``multiple`` is 1 which rounds to the nearest integer.
+  multiple has to be a positive value and can be casted to input type.  
+  For example, 100 corresponds to ounding to the nearest multiple of 100 


ounding -> rounding

EpsilonPrime · 2023-06-30T22:56:21Z

docs/source/cpp/compute.rst

+  which rounds to the nearest integer. For integer inputs a non-negative 
+  ``ndigits`` value is ignored and the input is returned unchanged. For integer
+  inputs, if ``-ndigits`` is larger than the maximum number of digits the 
+  input type can hold, it is truncated to the maximum digit. For example, 


maximum ndigits that the type can handle

EpsilonPrime · 2023-06-30T23:03:27Z

docs/source/cpp/compute.rst

+  ``ndigits`` value is ignored and the input is returned unchanged. For integer
+  inputs, if ``-ndigits`` is larger than the maximum number of digits the 
+  input type can hold, it is truncated to the maximum digit. For example, 
+  ``round([123], ndigits=-4, round_mode=DOWN)`` returns [100] for ``int8`` type.


There are three potential ways of handling this particular behavior (none of which are specified in the Substrait specification):

reject the operation as invalid

using the provided value (always returning overflow)

using the provided value (always returning max value)

fixing the value and proceeding

I am going to check other engines to see what they do in this particular case but the precedent within arrow seems to be to reject the operation as an overflow would occur (RoundToMultiple does this).

I agree that we should align with RoundToMultiple. I changed the behavior to rejecting the operation when -ndigits is too large.

EpsilonPrime · 2023-07-01T00:32:38Z

docs/source/cpp/compute.rst

+  ``ndigits`` value is ignored and the input is returned unchanged. For integer
+  inputs, if ``-ndigits`` is larger than the maximum number of digits the 
+  input type can hold, it is truncated to the maximum digit. For example, 
+  ``round([123], ndigits=-4, round_mode=DOWN)`` returns [100] for ``int8`` type.


(as the ndigits value is silently reduced to -2)

EpsilonPrime · 2023-07-01T01:09:47Z

I ran the benchmarks as well (locally on an M1 Macbook Pro):
archery benchmark diff review36289 main --benchmark-filter=RoundArrayBenchmark

The summary version is:

int8, uint8 130-1200x faster
int16, uint16 100-870x faster
int32, uint32 70-830x faster
int64, uint64 100-700x faster
float/double no noticible effect

https://gist.github.com/EpsilonPrime/658c90020a5964064e803cfb7e4761b2

js8544 · 2023-07-11T03:29:00Z

@EpsilonPrime Sorry for the delay, I've updated the PR according to your suggestions. Please re-review it when it's convenient for you. Thanks!

EpsilonPrime · 2023-07-11T03:35:20Z

The changes look great to me. Thanks for implementing this!

westonpace

Can you quickly comment on whether you agree this is a breaking change or not? Then I think we can approve this PR.

westonpace · 2023-07-11T16:17:23Z

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

+  template <typename T>
+  static enable_if_integer_value<T> Pow10(int64_t power) {
+    DCHECK_GE(power, 0);
+    DCHECK_LE(power, std::numeric_limits<T>::digits10);
+    static constexpr uint64_t lut[] = {
+        Pow10Struct<0>::value,  Pow10Struct<1>::value,  Pow10Struct<2>::value,
+        Pow10Struct<3>::value,  Pow10Struct<4>::value,  Pow10Struct<5>::value,
+        Pow10Struct<6>::value,  Pow10Struct<7>::value,  Pow10Struct<8>::value,
+        Pow10Struct<9>::value,  Pow10Struct<10>::value, Pow10Struct<11>::value,
+        Pow10Struct<12>::value, Pow10Struct<13>::value, Pow10Struct<14>::value,
+        Pow10Struct<15>::value, Pow10Struct<16>::value, Pow10Struct<17>::value,
+        Pow10Struct<18>::value, Pow10Struct<19>::value};
+
+    return static_cast<T>(lut[power]);
+  }
 };


This is clever, though I don't know if it is more readable than something like https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/decimal_internal.h#L36-L58

It does seem to be an overkill indeed. I've changed it to the simpler way.

westonpace · 2023-07-11T17:16:42Z

docs/source/cpp/compute.rst

@@ -563,30 +563,32 @@ representation based on the rounding criterion.
 +-------------------+------------+-------------+-------------------------+----------------------------------+--------+
 | floor             | Unary      | Numeric     | Float32/Float64/Decimal |                                  |        |
 +-------------------+------------+-------------+-------------------------+----------------------------------+--------+
-| round             | Unary      | Numeric     | Float32/Float64/Decimal | :struct:`RoundOptions`           | (1)(2) |
+| round             | Unary      | Numeric     | Input Type              | :struct:`RoundOptions`           | (1)(2) |


So I think this is technically a breaking change right?

Before, if we had something like:

x = pa.array([1, 2], pa.int32()) y = pc.round(x)

Then y would be a double array. Now, y will be an int32 array. I think this is correct and the old behavior was unintentional so I think it is an ok breaking change. Still, we should make sure to mark the PR as a breaking change if my understanding is correct so that we document it as such in the release notes.

CC @jorisvandenbossche for second opinion.

Right, it should be a breaking change. @jorisvandenbossche could you please confirm if it's acceptable?

Yes, fully agreed with the summary of Weston above: this was unintentional behaviour (because of automatic casting for numeric types), and it's fine to correct this with a breaking change.

docs/source/cpp/compute.rst

Co-authored-by: Weston Pace <[email protected]>

jorisvandenbossche · 2023-07-19T10:27:11Z

docs/source/cpp/compute.rst

@@ -563,30 +563,32 @@ representation based on the rounding criterion.
 +-------------------+------------+-------------+-------------------------+----------------------------------+--------+
 | floor             | Unary      | Numeric     | Float32/Float64/Decimal |                                  |        |


I would have expected that "floor" kernel is a small wrapper around "round" with a specific RoundOptions value. If the output type of "round" changes, that doesn't also change "floor"?

Not really. Because I didn't add floor kernels for integer types, floor(int) would still be dispatched to floor(float) kernels, and thus calling the round functions for floats.

But now that Round itself supports rounding integers, how hard would it be to expand the floor/trunc registration to integer types as well?
(like MakeUnaryRoundFunction was updated to loop through all NumericTypes instead of just float32/float64)

Is it at all useful to expose those functions for integer inputs?

Just to have consistent behaviour with the generic round (i.e. always preserve the input type). But that alone is maybe not worth it.

Although it might also simplify things, given that the RoundIntegerToFloatingPointFunction to explicitly cast int to float which is still used for floor/trunc/ceil could then be removed (but didn't check the code in detail)

#36786 I've created another issue for this.

js8544 · 2023-07-27T06:52:53Z

I think this PR is ready to be merged. CI failures are unrelated. @pitrou Would you mind merging this? Thanks!

pitrou · 2023-07-27T07:31:03Z

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

+  // Test different rounding mode
+  // skip int8 because of its small range
+  if constexpr (!std::is_same_v<TypeParam, Int8Type>) {
+    std::string values("[0, 1, -13, -50, 115, -176, 200, 250]");


Can you test with values on which HALF_TOWARDS_ZERO and HALF_TO_EVEN would actually differ?
For example:

Suggested change

std::string values("[0, 1, -13, -50, 115, -176, 200, 250]");

std::string values("[0, 1, -13, 115, -150, -176, 200, 250]");

I kept -50 and added -150, so that no two options result in identical results. (Changing -50 to -150 would make HALF_TO_ODD and HALF_UP the same).

pitrou · 2023-07-27T07:33:18Z

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

+  }
+
+  // An overly large ndigits would cause an error
+  if constexpr (std::is_same_v<TypeParam, Int8Type>) {


Why only int8? 100 digits should be out of range for every integer type.

Right. The if constexpr is removed.

pitrou · 2023-07-27T07:34:29Z

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

+  // Test different rounding mode
+  // skip uint8 because of its small range
+  if constexpr (!std::is_same_v<TypeParam, UInt8Type>) {
+    std::string values("[0, 1, 13, 50, 115, 176, 200, 250]");


(same comments as above here)

pitrou · 2023-07-27T07:39:56Z

docs/source/cpp/compute.rst

 +-------------------+------------+-------------+-------------------------+----------------------------------+--------+
-| round_to_multiple | Unary      | Numeric     | Float32/Float64/Decimal | :struct:`RoundToMultipleOptions` | (1)(3) |
+| round_to_multiple | Unary      | Numeric     | Input Type              | :struct:`RoundToMultipleOptions` | (1)(3) |


Is round_binary not mentioned in this table? If so, can you add it?

js8544 · 2023-08-09T12:18:39Z

friendly ping :)

pitrou

Thanks for the update, just two nits

docs/source/cpp/compute.rst

pitrou · 2023-08-09T14:21:01Z

docs/source/cpp/compute.rst

@@ -627,8 +635,8 @@ The example values are given for default values of ``ndigits`` and ``multiple``.
 +-----------------------+--------------------------------------------------------------+---------------------------+

 The following table gives examples of how ``ndigits`` (for the ``round``
-function) and ``multiple`` (for ``round_to_multiple``) influence the operance
-performed, respectively.
+function) and ``multiple`` (for ``round_to_multiple`` and ``round_binary``) 


Hmm, the second input for round_binary is equivalent to ndigits, not multiple, right?

My bad, fixed.

Co-authored-by: Antoine Pitrou <[email protected]>

conbench-apache-arrow · 2023-08-13T02:06:18Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 7c8f398.

There were 7 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-08-09 21:32:14Z
- file-read (R) with compression=lz4, dataset=fanniemae_2016Q4, file_type=feather, language=R, output_type=dataframe
- file-read (R) with compression=uncompressed, dataset=nyctaxi_2010-01, file_type=feather, language=R, output_type=table
and 5 more (see the report linked below)

The full Conbench report has more details.

### Rationale for this change Currently `round` casts integers to floats which causes undesired behavior. ### What changes are included in this PR? Add round kernels for integer types. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#35273 Lead-authored-by: Jin Shang <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Weston Pace <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

js8544 added 4 commits June 22, 2023 13:32

add integer round kernels

6fc5dc3

lint

063e73e

fix warning

f1a2729

fix existing tests

627cc1a

js8544 requested a review from westonpace as a code owner June 25, 2023 11:03

github-actions bot added Component: C++ Component: Documentation awaiting review Awaiting review labels Jun 25, 2023

js8544 requested a review from AlenkaF as a code owner June 25, 2023 15:24

github-actions bot added the Component: Python label Jun 25, 2023

EpsilonPrime reviewed Jul 1, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 1, 2023

js8544 added 2 commits July 6, 2023 17:52

pr feedback

eb5a739

add overflow test cast

de3c289

westonpace requested changes Jul 11, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 11, 2023

integer round use naive approach

89fc535

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 17, 2023

js8544 and others added 2 commits July 17, 2023 14:44

Update docs/source/cpp/compute.rst

4b4e85a

Co-authored-by: Weston Pace <[email protected]>

lint

f1f35f0

js8544 requested a review from westonpace July 17, 2023 06:45

AlenkaF removed their request for review July 17, 2023 08:29

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 19, 2023

jorisvandenbossche reviewed Jul 19, 2023

View reviewed changes

js8544 mentioned this pull request Jul 20, 2023

[C++][Python] Remove integer compatibility for trunc, floor and ceil #36786

Open

westonpace approved these changes Jul 25, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Jul 25, 2023

review feedback

f0c2eae

js8544 added the Breaking Change Includes a breaking change to the API label Jul 27, 2023

pitrou reviewed Jul 27, 2023

View reviewed changes

js8544 requested a review from pitrou July 28, 2023 02:18

pitrou requested changes Aug 9, 2023

View reviewed changes

js8544 and others added 2 commits August 9, 2023 22:22

Update docs/source/cpp/compute.rst

88d2619

Co-authored-by: Antoine Pitrou <[email protected]>

fix doc

0a8d38d

pitrou approved these changes Aug 9, 2023

View reviewed changes

pitrou merged commit 7c8f398 into apache:main Aug 9, 2023

pitrou removed the awaiting merge Awaiting merge label Aug 9, 2023

		@@ -563,30 +563,32 @@ representation based on the rounding criterion.
		+-------------------+------------+-------------+-------------------------+----------------------------------+--------+
		\| floor \| Unary \| Numeric \| Float32/Float64/Decimal \| \| \|

	std::string values("[0, 1, -13, -50, 115, -176, 200, 250]");
	std::string values("[0, 1, -13, 115, -150, -176, 200, 250]");

GH-35273: [C++] Add integer round kernels #36289

GH-35273: [C++] Add integer round kernels #36289

Conversation

js8544 commented Jun 25, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

westonpace commented Jun 29, 2023

EpsilonPrime commented Jun 30, 2023

EpsilonPrime commented Jun 30, 2023

js8544 commented Jun 30, 2023

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

EpsilonPrime commented Jul 1, 2023

js8544 commented Jul 11, 2023

EpsilonPrime commented Jul 11, 2023

westonpace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js8544 Jul 17, 2023 • edited Loading

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

Choose a reason for hiding this comment

js8544 commented Jul 27, 2023

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

js8544 commented Aug 9, 2023

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

conbench-apache-arrow bot commented Aug 13, 2023

js8544 commented Jun 25, 2023 •

edited by github-actions bot

Loading

js8544 Jul 17, 2023 •

edited

Loading