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.

Sign up for GitHub

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

[VL] Fix High Precision Rounding #6707

Merged
merged 5 commits into from
Aug 9, 2024
Merged

[VL] Fix High Precision Rounding #6707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bd03a3
Support Round for High Precision
ArnavBalyan Aug 4, 2024
f5ab0ed
lint
ArnavBalyan Aug 5, 2024
a093c25
format
ArnavBalyan Aug 5, 2024
668f497
fix test
ArnavBalyan Aug 6, 2024
9d5705e
address comments
ArnavBalyan Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions cpp/velox/operators/functions/Arithmetic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <folly/CPortability.h>
#include <stdint.h>
#include <cmath>
#include <limits>
#include <type_traits>

namespace gluten {
Expand All @@ -38,14 +39,16 @@ struct RoundFunction {
return number;
}

double factor = std::pow(10, decimals);
// Using long double for high precision during intermediate calculations.
// TODO: Make this more efficient with Boost to support high arbitrary precision at runtime.
long double factor = std::pow(10.0L, static_cast<long double>(decimals));
static const TNum kInf = std::numeric_limits<TNum>::infinity();

if (number < 0) {
return (std::round(std::nextafter(number, -kInf) * factor * -1) / factor) * -1;
return static_cast<TNum>((std::round(std::nextafter(number, -kInf) * factor * -1) / factor) * -1);
}
return std::round(std::nextafter(number, kInf) * factor) / factor;
return static_cast<TNum>(std::round(std::nextafter(number, kInf) * factor) / factor);
}

template <typename TInput>
FOLLY_ALWAYS_INLINE void call(TInput& result, const TInput& a, const int32_t b = 0) {
result = round(a, b);
Expand Down
3 changes: 3 additions & 0 deletions ...src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ class GlutenMathExpressionsSuite extends MathExpressionsSuite with GlutenTestsTr
checkEvaluation(Round(-3.5, 0), -4.0)
checkEvaluation(Round(-0.35, 1), -0.4)
checkEvaluation(Round(-35, -1), -40)
checkEvaluation(Round(1.12345678901234567, 8), 1.12345679)
checkEvaluation(Round(-0.98765432109876543, 5), -0.98765)
checkEvaluation(Round(12345.67890123456789, 6), 12345.678901)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if there is any reason for removing below case from the test. Is it due to some limitation?

checkEvaluation(Round(0.19324999999999998, 4), 0.1932)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized next_after can return different results on different machines or compilers. In this case, the intermediate round result is almost on the edge of being rounded to the next decimal (difference was almost negligible), so removed to prevent flakiness. I think we seem to be using double as an alternative to java bigdecimal at multiple places. We will probably eventually have to move to boost/mpfr otherwise might see difference in results compared to vanilla spark. I think it's not a big problem now, but maybe in the future. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying. I see your point.

I think we seem to be using double as an alternative to java bigdecimal at multiple places.

Just curious. Have you noticed other issues except for the round-related ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, so far only seen with round. I'll continue to explore if there are any other cases like this

checkEvaluation(BRound(2.5, 0), 2.0)
checkEvaluation(BRound(3.5, 0), 4.0)
checkEvaluation(BRound(-2.5, 0), -2.0)
Expand Down
4 changes: 4 additions & 0 deletions ...src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,10 @@ class GlutenMathExpressionsSuite extends MathExpressionsSuite with GlutenTestsTr
checkEvaluation(Round(-3.5, 0), -4.0)
checkEvaluation(Round(-0.35, 1), -0.4)
checkEvaluation(Round(-35, -1), -40)
checkEvaluation(Round(1.12345678901234567, 8), 1.12345679)
checkEvaluation(Round(-0.98765432109876543, 5), -0.98765)
checkEvaluation(Round(12345.67890123456789, 6), 12345.678901)
checkEvaluation(Round(-35, -1), -40)
checkEvaluation(Round(BigDecimal("45.00"), -1), BigDecimal(50))
checkEvaluation(BRound(2.5, 0), 2.0)
checkEvaluation(BRound(3.5, 0), 4.0)
Expand Down
3 changes: 3 additions & 0 deletions ...src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenMathExpressionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,9 @@ class GlutenMathExpressionsSuite extends MathExpressionsSuite with GlutenTestsTr
checkEvaluation(BRound(-3.5, 0), -4.0)
checkEvaluation(BRound(-0.35, 1), -0.4)
checkEvaluation(BRound(-35, -1), -40)
checkEvaluation(Round(1.12345678901234567, 8), 1.12345679)
checkEvaluation(Round(-0.98765432109876543, 5), -0.98765)
checkEvaluation(Round(12345.67890123456789, 6), 12345.678901)
checkEvaluation(BRound(BigDecimal("45.00"), -1), BigDecimal(40))
checkEvaluation(checkDataTypeAndCast(RoundFloor(Literal(2.5), Literal(0))), Decimal(2))
checkEvaluation(checkDataTypeAndCast(RoundFloor(Literal(3.5), Literal(0))), Decimal(3))
Expand Down
Loading