From 160a3c5f0b4803d5b9022655aba275a51f2abb6e Mon Sep 17 00:00:00 2001 From: Raza Jafri Date: Tue, 9 Jan 2024 13:46:30 -0800 Subject: [PATCH 1/7] Fixed decimal bounds Signed-off-by: Raza Jafri Added tests --- .../java/ai/rapids/cudf/DecimalUtils.java | 25 ++++++++---- .../java/ai/rapids/cudf/DecimalUtilsTest.java | 40 +++++++++++++++++++ 2 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java diff --git a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java index 1979bd1bd5b..cae2ccafb30 100644 --- a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java +++ b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java @@ -82,13 +82,15 @@ public static ColumnVector lessThan(ColumnView lhs, BigDecimal rhs) { int leftScale = lhs.getType().getScale(); int leftPrecision = lhs.getType().getDecimalMaxPrecision(); - // First we have to round the scalar (rhs) to the same scale as lhs. Because this is a - // less than and it is rhs that we are rounding, we will round away from 0 (UP) - // to make sure we always return the correct value. + // First we have to round the scalar (rhs) to the same scale as lhs. + // Because this is a less-than, and it is rhs that we are rounding, we will round away from 0 (UP) in case rhs is + // positive and toward 0 (DOWN) if rhs is negative to make sure we always return the correct value. // For example: - // 100.1 < 100.19 - // If we rounded down the rhs 100.19 would become 100.1, and now 100.1 is not < 100.1 - BigDecimal roundedRhs = rhs.setScale(-leftScale, BigDecimal.ROUND_UP); + // ex:1 100.1 < 100.19 + // ex:2 -100.2 < -100.19 + // In ex:1 If we rounded down the rhs 100.19 would become 100.1, and now 100.1 is not < 100.1 + // In ex:2 If we rounded up the rhs -100.19 would become -100.2, and now -100.2 is not < -100.2 + BigDecimal roundedRhs = rhs.setScale(-leftScale, rhs.signum() > 0 ? BigDecimal.ROUND_UP : BigDecimal.ROUND_DOWN); if (roundedRhs.precision() > leftPrecision) { // converting rhs to the same precision as lhs would result in an overflow/error, but @@ -142,7 +144,16 @@ public static ColumnVector greaterThan(ColumnView lhs, BigDecimal rhs) { // For example: // 100.2 > 100.19 // If we rounded up the rhs 100.19 would become 100.2, and now 100.2 is not > 100.2 - BigDecimal roundedRhs = rhs.setScale(-cvScale, BigDecimal.ROUND_DOWN); + + // First we have to round the scalar (rhs) to the same scale as lhs. + // Because this is a greater-than, and it is rhs that we are rounding, we will round towards 0 (DOWN) in case rhs is + // positive and away from 0 (UP) if rhs is negative to make sure we always return the correct value. + // For example: + // ex:1 100.2 > 100.19 + // ex:2 -100.1 > -100.19 + // In ex:1 If we rounded up the rhs 100.19 would become 100.2, and now 100.2 is not > 100.2 + // In ex:2 If we rounded down the rhs -100.19 would become -100.1, and now -100.1 is not > -100.1 + BigDecimal roundedRhs = rhs.setScale(-cvScale, rhs.signum() > 0 ? BigDecimal.ROUND_DOWN : BigDecimal.ROUND_UP); if (roundedRhs.precision() > maxPrecision) { // converting rhs to the same precision as lhs would result in an overflow/error, but diff --git a/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java b/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java new file mode 100644 index 00000000000..505a1772bd1 --- /dev/null +++ b/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java @@ -0,0 +1,40 @@ +/* + * + * Copyright (c) 2024, NVIDIA CORPORATION. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package ai.rapids.cudf; + +import org.junit.jupiter.api.Test; + +import java.math.BigDecimal; +import static ai.rapids.cudf.AssertUtils.assertColumnsAreEqual; + +public class DecimalUtilsTest extends CudfTestBase { + @Test + public void testOutOfBounds() { + try (ColumnView cv = ColumnVector.fromDecimals( + new BigDecimal("-1E+3"), + new BigDecimal("1E+3"), + new BigDecimal("9E+1"), + new BigDecimal("-9E+1"), + new BigDecimal("-91")); + ColumnView expected = ColumnVector.fromBooleans(true, true, false, false, true)) { + ColumnView result = DecimalUtils.outOfBounds(cv, 1, -1); + assertColumnsAreEqual(expected, result); + } + } +} \ No newline at end of file From c1777797a08f5f2b628d23cccdf2608c78ab2b2d Mon Sep 17 00:00:00 2001 From: Raza Jafri Date: Tue, 9 Jan 2024 18:24:33 -0800 Subject: [PATCH 2/7] ran pre-commit --- java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java b/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java index 505a1772bd1..5ab3e682b97 100644 --- a/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java +++ b/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java @@ -37,4 +37,4 @@ public void testOutOfBounds() { assertColumnsAreEqual(expected, result); } } -} \ No newline at end of file +} From 7d70200f8f049bf3bb5499f3695d19cee3357dce Mon Sep 17 00:00:00 2001 From: Raza Jafri Date: Tue, 9 Jan 2024 19:27:02 -0800 Subject: [PATCH 3/7] close result vector --- java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java b/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java index 5ab3e682b97..a96eeda5dd7 100644 --- a/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java +++ b/java/src/test/java/ai/rapids/cudf/DecimalUtilsTest.java @@ -32,8 +32,8 @@ public void testOutOfBounds() { new BigDecimal("9E+1"), new BigDecimal("-9E+1"), new BigDecimal("-91")); - ColumnView expected = ColumnVector.fromBooleans(true, true, false, false, true)) { - ColumnView result = DecimalUtils.outOfBounds(cv, 1, -1); + ColumnView expected = ColumnVector.fromBooleans(true, true, false, false, true); + ColumnView result = DecimalUtils.outOfBounds(cv, 1, -1)) { assertColumnsAreEqual(expected, result); } } From 3f00b4591af413a02794bbacc5b60730ea5d4265 Mon Sep 17 00:00:00 2001 From: Raza Jafri Date: Wed, 10 Jan 2024 09:35:34 -0800 Subject: [PATCH 4/7] addressed review comments --- .../java/ai/rapids/cudf/DecimalUtils.java | 35 +++++++------------ 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java index cae2ccafb30..944fe7623ae 100644 --- a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java +++ b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java @@ -83,14 +83,12 @@ public static ColumnVector lessThan(ColumnView lhs, BigDecimal rhs) { int leftPrecision = lhs.getType().getDecimalMaxPrecision(); // First we have to round the scalar (rhs) to the same scale as lhs. - // Because this is a less-than, and it is rhs that we are rounding, we will round away from 0 (UP) in case rhs is - // positive and toward 0 (DOWN) if rhs is negative to make sure we always return the correct value. - // For example: - // ex:1 100.1 < 100.19 - // ex:2 -100.2 < -100.19 - // In ex:1 If we rounded down the rhs 100.19 would become 100.1, and now 100.1 is not < 100.1 - // In ex:2 If we rounded up the rhs -100.19 would become -100.2, and now -100.2 is not < -100.2 - BigDecimal roundedRhs = rhs.setScale(-leftScale, rhs.signum() > 0 ? BigDecimal.ROUND_UP : BigDecimal.ROUND_DOWN); + // For comparing the two values they should be the same scale, we round the value to positive infinity to maintain + // the relation. Ex: + // 10.2 < 10.29 = true, after rounding rhs to ceiling ===> 10.2 < 10.3 = true, relation is maintained + // 10.3 < 10.29 = false, after rounding rhs to ceiling ===> 10.3 < 10.3 = false, relation is maintained + // 10.1 < 10.10 = false, after rounding rhs to ceiling ===> 10.1 < 10.1 = false, relation is maintained + BigDecimal roundedRhs = rhs.setScale(-leftScale, BigDecimal.ROUND_CEILING); if (roundedRhs.precision() > leftPrecision) { // converting rhs to the same precision as lhs would result in an overflow/error, but @@ -138,22 +136,13 @@ public static ColumnVector greaterThan(ColumnView lhs, BigDecimal rhs) { int cvScale = lhs.getType().getScale(); int maxPrecision = lhs.getType().getDecimalMaxPrecision(); - // First we have to round the scalar (rhs) to the same scale as lhs. Because this is a - // greater than and it is rhs that we are rounding, we will round towards 0 (DOWN) - // to make sure we always return the correct value. - // For example: - // 100.2 > 100.19 - // If we rounded up the rhs 100.19 would become 100.2, and now 100.2 is not > 100.2 - // First we have to round the scalar (rhs) to the same scale as lhs. - // Because this is a greater-than, and it is rhs that we are rounding, we will round towards 0 (DOWN) in case rhs is - // positive and away from 0 (UP) if rhs is negative to make sure we always return the correct value. - // For example: - // ex:1 100.2 > 100.19 - // ex:2 -100.1 > -100.19 - // In ex:1 If we rounded up the rhs 100.19 would become 100.2, and now 100.2 is not > 100.2 - // In ex:2 If we rounded down the rhs -100.19 would become -100.1, and now -100.1 is not > -100.1 - BigDecimal roundedRhs = rhs.setScale(-cvScale, rhs.signum() > 0 ? BigDecimal.ROUND_DOWN : BigDecimal.ROUND_UP); + // For comparing the two values they should be the same scale, we round the value to negative infinity to maintain + // the relation. Ex: + // 10.3 > 10.29 = true, after rounding rhs to floor ===> 10.3 > 10.2 = true, relation is maintained + // 10.2 > 10.29 = false, after rounding rhs to floor ===> 10.2 < 10.2 = false, relation is maintained + // 10.1 > 10.10 = false, after rounding rhs to floor ===> 10.1 < 10.1 = false, relation is maintained + BigDecimal roundedRhs = rhs.setScale(-cvScale, BigDecimal.ROUND_FLOOR); if (roundedRhs.precision() > maxPrecision) { // converting rhs to the same precision as lhs would result in an overflow/error, but From 1ff00de18a7838a5030f4a346745d821a2af158f Mon Sep 17 00:00:00 2001 From: Raza Jafri Date: Wed, 10 Jan 2024 09:36:10 -0800 Subject: [PATCH 5/7] updated copyrights --- java/src/main/java/ai/rapids/cudf/DecimalUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java index 944fe7623ae..53320e3b242 100644 --- a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java +++ b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2022, NVIDIA CORPORATION. + * Copyright (c) 2024, NVIDIA CORPORATION. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From 1eb3c28637a0d76797d602970f0b66a65bfc6bac Mon Sep 17 00:00:00 2001 From: Raza Jafri Date: Wed, 10 Jan 2024 09:44:35 -0800 Subject: [PATCH 6/7] Update java/src/main/java/ai/rapids/cudf/DecimalUtils.java Co-authored-by: Jason Lowe --- java/src/main/java/ai/rapids/cudf/DecimalUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java index 53320e3b242..6cbf1d66953 100644 --- a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java +++ b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2024, NVIDIA CORPORATION. + * Copyright (c) 2022-2024, NVIDIA CORPORATION. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From 62c3f85046f46ccd49b049cc2236fd3d4fbcc724 Mon Sep 17 00:00:00 2001 From: Raza Jafri Date: Wed, 10 Jan 2024 09:47:53 -0800 Subject: [PATCH 7/7] updated comments --- java/src/main/java/ai/rapids/cudf/DecimalUtils.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java index 6cbf1d66953..7a5be9b08b9 100644 --- a/java/src/main/java/ai/rapids/cudf/DecimalUtils.java +++ b/java/src/main/java/ai/rapids/cudf/DecimalUtils.java @@ -140,8 +140,8 @@ public static ColumnVector greaterThan(ColumnView lhs, BigDecimal rhs) { // For comparing the two values they should be the same scale, we round the value to negative infinity to maintain // the relation. Ex: // 10.3 > 10.29 = true, after rounding rhs to floor ===> 10.3 > 10.2 = true, relation is maintained - // 10.2 > 10.29 = false, after rounding rhs to floor ===> 10.2 < 10.2 = false, relation is maintained - // 10.1 > 10.10 = false, after rounding rhs to floor ===> 10.1 < 10.1 = false, relation is maintained + // 10.2 > 10.29 = false, after rounding rhs to floor ===> 10.2 > 10.2 = false, relation is maintained + // 10.1 > 10.10 = false, after rounding rhs to floor ===> 10.1 > 10.1 = false, relation is maintained BigDecimal roundedRhs = rhs.setScale(-cvScale, BigDecimal.ROUND_FLOOR); if (roundedRhs.precision() > maxPrecision) {