This is an automated email from the ASF dual-hosted git repository.
rui pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/incubator-gluten.git
The following commit(s) were added to refs/heads/main by this push:
new 1198e2d0d [GLUTEN-7323][VL] Always round negative decimals for
integral types (#7337)
1198e2d0d is described below
commit 1198e2d0d949b6dc87f84af62d28b4d622948b96
Author: surnaik <[email protected]>
AuthorDate: Wed Sep 25 15:00:12 2024 +0530
[GLUTEN-7323][VL] Always round negative decimals for integral types (#7337)
---
.../gluten/execution/ScalarFunctionsValidateSuite.scala | 7 +++++++
cpp/velox/operators/functions/Arithmetic.h | 11 ++++++++++-
cpp/velox/tests/SparkFunctionTest.cc | 4 ++--
.../sql/catalyst/expressions/GlutenMathExpressionsSuite.scala | 2 ++
.../sql/catalyst/expressions/GlutenMathExpressionsSuite.scala | 2 ++
.../sql/catalyst/expressions/GlutenMathExpressionsSuite.scala | 2 ++
.../sql/catalyst/expressions/GlutenMathExpressionsSuite.scala | 2 ++
7 files changed, 27 insertions(+), 3 deletions(-)
diff --git
a/backends-velox/src/test/scala/org/apache/gluten/execution/ScalarFunctionsValidateSuite.scala
b/backends-velox/src/test/scala/org/apache/gluten/execution/ScalarFunctionsValidateSuite.scala
index ecad62984..05ef86e5e 100644
---
a/backends-velox/src/test/scala/org/apache/gluten/execution/ScalarFunctionsValidateSuite.scala
+++
b/backends-velox/src/test/scala/org/apache/gluten/execution/ScalarFunctionsValidateSuite.scala
@@ -1410,4 +1410,11 @@ abstract class ScalarFunctionsValidateSuite extends
FunctionsValidateSuite {
}
}
}
+
+ test("round on integral types should return same values as spark") {
+ // Scale > 0 should return same value as input on integral values
+ compareResultsAgainstVanillaSpark("select round(78, 1)", true, { _ => })
+ // Scale < 0 should round down even on integral values
+ compareResultsAgainstVanillaSpark("select round(44, -1)", true, { _ => })
+ }
}
diff --git a/cpp/velox/operators/functions/Arithmetic.h
b/cpp/velox/operators/functions/Arithmetic.h
index 7b4c9ae9d..9b2e22ac4 100644
--- a/cpp/velox/operators/functions/Arithmetic.h
+++ b/cpp/velox/operators/functions/Arithmetic.h
@@ -22,8 +22,17 @@
namespace gluten {
template <typename T>
+/// Round function
+/// When AlwaysRoundNegDec is true, spark semantics is followed which
+/// rounds negative decimals for integrals and does not round it otherwise
+/// Note that is is likely techinically impossible for this function to return
+/// expected results in all cases as the loss of precision plagues it on both
+/// paths: factor multiplication for large numbers and addition of truncated
+/// number to the rounded fraction for small numbers.
+/// We are trying to minimize the loss of precision by using the best path for
+/// the number, but the journey is likely not over yet.
struct RoundFunction {
- template <typename TNum, typename TDecimals, bool alwaysRoundNegDec = false>
+ template <typename TNum, typename TDecimals, bool alwaysRoundNegDec = true>
FOLLY_ALWAYS_INLINE TNum round(const TNum& number, const TDecimals& decimals
= 0) {
static_assert(!std::is_same_v<TNum, bool> && "round not supported for
bool");
diff --git a/cpp/velox/tests/SparkFunctionTest.cc
b/cpp/velox/tests/SparkFunctionTest.cc
index a340b4fca..2105b155e 100644
--- a/cpp/velox/tests/SparkFunctionTest.cc
+++ b/cpp/velox/tests/SparkFunctionTest.cc
@@ -87,9 +87,9 @@ class SparkFunctionTest : public SparkFunctionBaseTest {
{1, 10, 1},
{0, 10, 0},
{-1, 10, -1},
- {1, -1, 1},
+ {1, -1, 0},
{0, -2, 0},
- {-1, -3, -1}};
+ {-1, -3, 0}};
}
};
diff --git
a/gluten-ut/spark32/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
b/gluten-ut/spark32/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
index 5e58fcb62..1f343e1bb 100644
---
a/gluten-ut/spark32/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
+++
b/gluten-ut/spark32/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
@@ -124,6 +124,8 @@ class GlutenMathExpressionsSuite extends
MathExpressionsSuite with GlutenTestsTr
checkEvaluation(Round(1.12345678901234567, 8), 1.12345679)
checkEvaluation(Round(-0.98765432109876543, 5), -0.98765)
checkEvaluation(Round(12345.67890123456789, 6), 12345.678901)
+ checkEvaluation(Round(44, -1), 40)
+ checkEvaluation(Round(78, 1), 78)
// Enable the test after fixing
https://github.com/apache/incubator-gluten/issues/6827
// checkEvaluation(Round(0.5549999999999999, 2), 0.55)
checkEvaluation(BRound(2.5, 0), 2.0)
diff --git
a/gluten-ut/spark33/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
b/gluten-ut/spark33/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
index a8716b6ef..e4c59095e 100644
---
a/gluten-ut/spark33/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
+++
b/gluten-ut/spark33/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
@@ -288,6 +288,8 @@ class GlutenMathExpressionsSuite extends
MathExpressionsSuite with GlutenTestsTr
// Enable the test after fixing
https://github.com/apache/incubator-gluten/issues/6827
// checkEvaluation(Round(0.5549999999999999, 2), 0.55)
checkEvaluation(Round(-35, -1), -40)
+ checkEvaluation(Round(44, -1), 40)
+ checkEvaluation(Round(78, 1), 78)
checkEvaluation(Round(BigDecimal("45.00"), -1), BigDecimal(50))
checkEvaluation(BRound(2.5, 0), 2.0)
checkEvaluation(BRound(3.5, 0), 4.0)
diff --git
a/gluten-ut/spark34/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
b/gluten-ut/spark34/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
index 96c53306b..826176334 100644
---
a/gluten-ut/spark34/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
+++
b/gluten-ut/spark34/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
@@ -242,6 +242,8 @@ class GlutenMathExpressionsSuite extends
MathExpressionsSuite with GlutenTestsTr
checkEvaluation(Round(-0.35, 1), -0.4)
checkEvaluation(Round(-35, -1), -40)
checkEvaluation(Round(BigDecimal("45.00"), -1), BigDecimal(50))
+ checkEvaluation(Round(44, -1), 40)
+ checkEvaluation(Round(78, 1), 78)
checkEvaluation(BRound(2.5, 0), 2.0)
checkEvaluation(BRound(3.5, 0), 4.0)
checkEvaluation(BRound(-2.5, 0), -2.0)
diff --git
a/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
b/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
index 5196e760f..d49bbd355 100644
---
a/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
+++
b/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
@@ -242,6 +242,8 @@ class GlutenMathExpressionsSuite extends
MathExpressionsSuite with GlutenTestsTr
checkEvaluation(Round(-0.35, 1), -0.4)
checkEvaluation(Round(-35, -1), -40)
checkEvaluation(Round(BigDecimal("45.00"), -1), BigDecimal(50))
+ checkEvaluation(Round(44, -1), 40)
+ checkEvaluation(Round(78, 1), 78)
checkEvaluation(BRound(2.5, 0), 2.0)
checkEvaluation(BRound(3.5, 0), 4.0)
checkEvaluation(BRound(-2.5, 0), -2.0)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
