ARROW-4206: [Gandiva] support decimal divide and mod #3813
Conversation
|
@pravindra, add me as a reviewer, @pitrou is not available. |
|
the "review requested" feature is only informational, I think @pravindra is one of the only contributors who uses it regularly =) |
sorry I'm not able to add you as a reviewer. but, please do review. |
cpp/src/gandiva/decimal_xlarge.cc
Outdated
| DCHECK_LE(scale, kMaxScaleMultiplier); | ||
|
|
||
| // Compute the scale multipliers once. | ||
| static std::array<int256_t, kMaxScaleMultiplier + 1> multipliers = |
There was a problem hiding this comment.
This is not thread safe.
There was a problem hiding this comment.
The spec says it's thread-safe https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables
Is there an exception for lambda ?
There was a problem hiding this comment.
I've made this a global variable now.
There was a problem hiding this comment.
There doesn't seem to be exception for lamda, I didn't know that local static storage was thread safe in C++11, wasn't in C11 and previous C++ versions. I'd say you can keep the previous version if you prefer.
cpp/src/gandiva/decimal_xlarge.cc
Outdated
| ([]() -> std::array<int256_t, kMaxScaleMultiplier + 1> { | ||
| std::array<int256_t, kMaxScaleMultiplier + 1> values; | ||
| values[0] = 1; | ||
| for (auto idx = 1; idx <= kMaxScaleMultiplier; idx++) { |
There was a problem hiding this comment.
Don't use auto on primitive types.
There was a problem hiding this comment.
curious, why the restriction ? is this from the coding standard or for efficiency ?
There was a problem hiding this comment.
Mostly for readability (and future refactoring), and making sure there's no signedness/width issues, though the compiler would help with the warnings.
cpp/src/arrow/util/basic_decimal.h
Outdated
| @@ -138,6 +138,9 @@ class ARROW_EXPORT BasicDecimal128 { | |||
| /// - If 'round' is false, the right-most digits are simply dropped. | |||
| BasicDecimal128 ReduceScaleBy(int32_t reduce_by, bool round = true) const; | |||
|
|
|||
| // returns 1 for positive and zero decimal values, -1 for negative decimal values. | |||
| int64_t Sign() const; | |||
There was a problem hiding this comment.
small enough to make it inline.
cpp/src/arrow/util/decimal-test.cc
Outdated
| @@ -482,6 +482,49 @@ TEST(Decimal128Test, Multiply) { | |||
| ASSERT_EQ(result.ToIntegerString(), "60501"); | |||
| } | |||
|
|
|||
| TEST(Decimal128Test, Divide) { | |||
| Decimal128 result; | |||
There was a problem hiding this comment.
This is valid for the next 3 tests:
- Why the usage of a temporary variables, e.g.
ASSERT_EQ(Decimal128("20100") / Decimal128("301"), ...)? - Why the usage of
ToIntegerString, just compareDecimal("66")? - Would it be better to explicit some int64/int128 constructors such that you can at least call with
Decimal(66)?
The end would look like
ASSERT_EQ(Decimal(20100) / Decimal128(301), Decimal128(66));
- Please do some minimal property testing against known types, e.g.
for (x: rand_int32_range())
for (y: rand_int32_range())
ASSERT_EQ(Decimal(x) / Decimal128(y), Decimal128(x/y));
- I usually try to test all obvious edge cases and some close bounds, e.g.
{INT_MIN, 0, INT_MAX} x {-2,-1,0,1,2}
There was a problem hiding this comment.
The int64/int32 constructors already exist. I've added tests and fixed as suggested.
| int256_t result_large = x_large_scaled_up / y_large; | ||
| int256_t remainder_large = x_large_scaled_up % y_large; | ||
|
|
||
| if (abs(2 * remainder_large) >= abs(y_large)) { |
There was a problem hiding this comment.
I don't follow, please comment.
| } | ||
|
|
||
| // scale upto the output scale, and do an integer division. | ||
| auto delta_scale = out_scale + y.scale() - x.scale(); |
There was a problem hiding this comment.
Don't use auto on primitive types.
| decimalops::Divide(reinterpret_cast<int64>(&context), DecimalScalar128{"201", 20, 3}, | ||
| DecimalScalar128{"1", 20, 2}, result_precision, result_scale, | ||
| &overflow); | ||
| EXPECT_EQ(context.has_error(), false); |
There was a problem hiding this comment.
EXPECT_TRUE and EXPECT_FALSE exists for this purpose.
|
One last issue with the newer changes, why the usage of |
Yeah - it's due to lack of int128_t constructor. I don't want to add boost dependency in the decimal header files since it'll spill over to the IR code, and glib. Since this is required only for testing, I'd like to leave this as now. Can revisit later if we have other uses of this. I believe the gcc's int128_t doesn't work with windows. |
No description provided.