Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Remainder for Decimal128 that handles when operands overflow #1175

Merged
merged 3 commits into from
May 30, 2023

Conversation

NVnavkumar
Copy link
Collaborator

Part of a fix for NVIDIA/spark-rapids#8330.

This implements a native GPU implementation of the Remainder computation for 2 DECIMAL128 columns. This patterns the same algorithm that the JDK uses when computing the remainder for 2 BigDecimal values (which is the algorithm that Apache Spark relies on). This is used when trying the compute the remainder for 2 columns that would otherwise need to be upconverted to a higher precision than is supported by the Decimal128 format (higher than 38).

The algorithm is quite simple for a % b (where a and b are 2 DECIMAL128 values/columns):

  1. Compute the integer division of the 2 columns: r_i = a // b.
  2. Multiply that result by b: a_int = r_i * b.
  3. The remainder is the result of subtracting that value from a: r = a - a_int.

This PR does this in a single custom kernel for optimization, and returns a result in the requested scale.

A corresponding PR in https://github.com/NVIDIA/spark-rapids/ will be created to implement the Spark plugin support for this.

For n % d
remainder = n - (n div d) * d

Signed-off-by: Navin Kumar <[email protected]>
Signed-off-by: Navin Kumar <[email protected]>
@NVnavkumar
Copy link
Collaborator Author

build

@NVnavkumar NVnavkumar self-assigned this May 26, 2023
@NVnavkumar NVnavkumar requested review from razajafri and revans2 and removed request for razajafri May 26, 2023 17:32
@NVnavkumar NVnavkumar added the enhancement New feature or request label May 26, 2023
revans2
revans2 previously approved these changes May 26, 2023
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Looks good. Because it is so inefficient, it would be nice to have a follow on issue to see if we can speed this up. Or at least do a benchmark and see if it is good/bad.

@@ -58,6 +58,22 @@ JNIEXPORT jlongArray JNICALL Java_com_nvidia_spark_rapids_jni_DecimalUtils_divid
CATCH_STD(env, 0);
}

JNIEXPORT jlongArray JNICALL Java_com_nvidia_spark_rapids_jni_DecimalUtils_remainder128(JNIEnv *env, jclass,
Copy link
Collaborator

Choose a reason for hiding this comment

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

file needs copyright updated.

@@ -25,6 +25,9 @@
#include <cmath>
#include <cstddef>

#define D19_ UINT64_C(10000000000000000000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this used in this PR.

@NVnavkumar
Copy link
Collaborator Author

build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants