ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions #10350

anthonylouisbsb · 2021-05-17T19:26:31Z

Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC.

The RANDOM functions are already implemented in Gandiva.

github-actions · 2021-05-17T19:26:55Z

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

kiszk · 2021-05-19T08:37:10Z

cpp/src/gandiva/precompiled/extended_math_ops.cc

I am curious whether all platforms can convert 64-bit integer to long double without loss of digits. (i.e. size(long double) > size(int64)).

@kiszk I checked that by the IEE-754 convention, the numbers in double precision should use at least 8 bytes to construct their representation, so I think that there would not have any digit loss in conversion.

cpp/src/gandiva/precompiled/extended_math_ops_test.cc

cpp/src/gandiva/precompiled/extended_math_ops.cc

kiszk · 2021-05-19T08:50:28Z

cpp/src/gandiva/precompiled/extended_math_ops.cc

Can we ensure that the same semantics (shift logical or shift arithmetic) on all platforms using just >>?

@kiszk Thanks for your revision, I checked the C++ reference, and I noticed that the shift operators have undefined or implementation-choice behavior for signed and negative numbers.

So as in this Pull Request, we are implementing the Arithmetical Shift, we can cast the input parameters to their respective unsigned types and then apply the shift operators and they will have the same behavior in all platforms. Do you agree with the solution?

cpp/src/gandiva/precompiled/extended_math_ops_test.cc

kiszk · 2021-05-19T11:59:57Z

#10274 handles abs operation well.

anthonylouisbsb · 2021-05-20T13:05:32Z

I included the CEIL function in the title because it was implemented too in Pull Request

kiszk · 2021-05-21T07:25:47Z

cpp/src/gandiva/gdv_function_stubs_test.cc

Does it mean that we will accept the loss of digits in INT64?

@kiszk Thanks for your revision! Please, could you explain how would occur digit loss here? I did not understand what you said.

Sorry, I thought the previous version of abs is to cast int64 to double long. But, now, gdv_fn_abs_int64 handles as int64. Correct?

@kiszk Yes, this line has the changes I made in the function. I handle the number as int64 and I add a check similar to the compute kernel to avoid overflow when getting the absolute value for the INT64_MIN value.

anthonylouisbsb · 2021-05-25T02:05:40Z

@kiszk I applied all suggestions you proposed, could you check if the corrections are fine? Thanks in advance for the review!

anthonylouisbsb · 2021-06-01T19:50:06Z

The LN function was added in this PR too.

jpedroantunes · 2021-06-03T10:51:23Z

The LOG2 function was added to the Pull Request

anthonylouisbsb · 2021-07-15T13:25:52Z

@kiszk @pitrou and @lidavidm I follow your suggestions and I changed the shift functions to have the same behavior as the Compute module. Thanks for your help!

@kiszk I also applied your suggestions to add more cases in the tests, when you be available, could you take another look in the changes?

cpp/src/gandiva/precompiled/extended_math_ops.cc

augustoasilva · 2021-07-16T00:58:39Z

cpp/src/gandiva/gdv_function_stubs_test.cc

Why did you choose this threshold? On fabs documentation it uses 6 places for decimal part, shouldn't it be 0.000005 a better value for AlmostEquals threshold?

I think choosing an adequate does not have a simple and unique answer like is shown in these articles:

https://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html

https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/

I choose that number because it is the value defined in the float header as default threshold . For huge and small numbers, I think the developer should set the threshold manually when he is calling the method.

I will change the method to use the FLT_EPSILON from the macro

anthonylouisbsb · 2021-09-14T07:15:09Z

@kiszk I applied all suggestions you made in the PR. Could you take another look at the PR and see if it is ready to be merged?

cpp/src/gandiva/function_registry_math_ops.cc

kiszk · 2021-10-15T06:56:50Z

Sorry for late review. It looks good to me. I would like to ask another review.
@pitrou Could you please review this?

anthonylouisbsb · 2021-11-23T17:29:31Z

@pitrou When you be free, could you take a look at this PR? I applied all changes that you and @kiszk suggested.

kszucs · 2021-12-06T14:29:09Z

ping @pitrou

anthonylouisbsb · 2022-01-10T22:13:50Z

@pitrou can you take a look at this PR? Because I want to close it as soon as possible

anthonylouisbsb · 2022-01-25T02:35:17Z

@pitrou can you take a look at it?

pitrou · 2022-01-25T09:32:22Z

I would rather if a Gandiva developer vetted this PR. @pravindra perhaps?

vvellanki · 2022-01-31T12:51:25Z

cpp/src/gandiva/precompiled/extended_math_ops.cc

What should happen if there is overflow?

overflow is an in-out parameter.. for some of the later calls overflow could be false - will that cause an issue?

@vvellanki I add a check for that case and it will throw an error if that situation occurs, check that commit: d25a630

amol- · 2023-03-30T17:22:24Z

Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍

github-actions bot added Component: Gandiva Component: C++ labels May 17, 2021

kiszk reviewed May 19, 2021

View reviewed changes

cpp/src/gandiva/precompiled/extended_math_ops_test.cc Outdated Show resolved Hide resolved

kiszk reviewed May 19, 2021

View reviewed changes

cpp/src/gandiva/precompiled/extended_math_ops.cc Outdated Show resolved Hide resolved

kiszk reviewed May 19, 2021

View reviewed changes

cpp/src/gandiva/precompiled/extended_math_ops_test.cc Outdated Show resolved Hide resolved

jpedroantunes force-pushed the feature/add-new-math-functions branch from 70399da to 124a58d Compare May 20, 2021 12:43

anthonylouisbsb changed the title ~~ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT and TRUNC functions~~ ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions May 20, 2021

kiszk reviewed May 21, 2021

View reviewed changes

anthonylouisbsb force-pushed the feature/add-new-math-functions branch 3 times, most recently from 6b7a038 to 1b0747b Compare May 25, 2021 02:04

anthonylouisbsb force-pushed the feature/add-new-math-functions branch 4 times, most recently from 198a993 to aeea171 Compare May 25, 2021 23:22

jpedroantunes force-pushed the feature/add-new-math-functions branch from aeea171 to 5d186be Compare May 26, 2021 12:50

anthonylouisbsb force-pushed the feature/add-new-math-functions branch 2 times, most recently from 9d4f95c to 1ac8f04 Compare May 31, 2021 18:54

jpedroantunes force-pushed the feature/add-new-math-functions branch from 1ac8f04 to a1f2aad Compare June 1, 2021 13:18

anthonylouisbsb changed the title ~~ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL and TRUNC functions~~ ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC and LN functions Jun 1, 2021

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from a1f2aad to 098861c Compare June 1, 2021 20:07

anthonylouisbsb changed the title ~~ARROW-12814: [C++][Gandiva] Implements the ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC and LN functions~~ ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions Jun 1, 2021

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from 33e97d9 to c242520 Compare July 15, 2021 16:41

augustoasilva reviewed Jul 16, 2021

View reviewed changes

anthonylouisbsb force-pushed the feature/add-new-math-functions branch 2 times, most recently from 9173083 to 848fe4e Compare July 21, 2021 21:38

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from 848fe4e to 0a521ef Compare August 6, 2021 20:52

anthonylouisbsb requested a review from kiszk September 28, 2021 17:44

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from 0a521ef to 7c74202 Compare September 28, 2021 17:45

kiszk reviewed Oct 2, 2021

View reviewed changes

cpp/src/gandiva/function_registry_math_ops.cc Outdated Show resolved Hide resolved

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from 7c74202 to a66a367 Compare October 5, 2021 13:38

anthonylouisbsb requested a review from kiszk October 6, 2021 17:42

kiszk approved these changes Oct 15, 2021

View reviewed changes

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from a66a367 to b94b1a1 Compare October 19, 2021 14:11

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from b94b1a1 to cc0e9fa Compare November 23, 2021 17:26

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from e6a1c05 to cb9ce06 Compare December 8, 2021 22:10

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from cb9ce06 to 59778bf Compare December 23, 2021 11:47

vvellanki reviewed Jan 31, 2022

View reviewed changes

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from 59778bf to 879a346 Compare February 2, 2022 22:21

jpedroantunes and others added 2 commits February 9, 2022 21:01

Add math functions

458b9bd

Add treatement when truncate overflow

9f450bf

anthonylouisbsb force-pushed the feature/add-new-math-functions branch from d25a630 to 9f450bf Compare February 10, 2022 00:03

amol- closed this Mar 30, 2023

ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions #10350

ARROW-12814: [C++][Gandiva] Implements ABS, FLOOR, PI, SQRT, SIGN, LSHIFT, RSHIFT, CEIL, TRUNC, LN and LOG2 functions #10350

Uh oh!

Conversation

anthonylouisbsb commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonylouisbsb May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kiszk commented May 19, 2021

Uh oh!

anthonylouisbsb commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

anthonylouisbsb May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonylouisbsb commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anthonylouisbsb commented Jun 1, 2021

Uh oh!

jpedroantunes commented Jun 3, 2021

Uh oh!

anthonylouisbsb commented Jul 15, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonylouisbsb Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonylouisbsb commented Sep 14, 2021

Uh oh!

Uh oh!

kiszk commented Oct 15, 2021

Uh oh!

anthonylouisbsb commented Nov 23, 2021

Uh oh!

kszucs commented Dec 6, 2021

Uh oh!

anthonylouisbsb commented Jan 10, 2022

Uh oh!

anthonylouisbsb commented Jan 25, 2022

Uh oh!

pitrou commented Jan 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amol- commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

anthonylouisbsb commented May 17, 2021 •

edited

Loading

anthonylouisbsb May 24, 2021 •

edited

Loading

anthonylouisbsb commented May 20, 2021 •

edited

Loading

anthonylouisbsb May 25, 2021 •

edited

Loading

anthonylouisbsb commented May 25, 2021 •

edited

Loading

anthonylouisbsb Jul 16, 2021 •

edited

Loading