-
Notifications
You must be signed in to change notification settings - Fork 915
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
[Java] Added a few more tests for Decimal to String cast #9818
Conversation
@@ -3376,16 +3376,27 @@ void testFixedWidthCast() { | |||
void testCastBigDecimalToString() { | |||
BigDecimal[] bigValues = {new BigDecimal("923121331938210123.321"), | |||
new BigDecimal("9223372036854775808.191"), | |||
new BigDecimal("-9.223"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a test case for a negative decimal value greater than 64 bits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing that will accomplish is to verify that Java BigDecimals > 64 bit have precision > 38
Codecov Report
@@ Coverage Diff @@
## branch-22.02 #9818 +/- ##
================================================
- Coverage 10.49% 10.44% -0.05%
================================================
Files 119 119
Lines 20305 20419 +114
================================================
+ Hits 2130 2133 +3
- Misses 18175 18286 +111
Continue to review full report at Codecov.
|
|
||
BigDecimal[] bigValues0 = {new BigDecimal("992983283728193827182918744829283742232")}; | ||
try { | ||
ColumnVector cv = ColumnVector.fromDecimals(bigValues0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make sure that this didn't leak?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously we should!
rerun tests |
if (cv != null) { | ||
cv.close(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not try(....cv = ..) {}
? It is auto-closed, isn't it?
@gpucibot merge |
This PR adds a few more edge cases as a sanity test on the request of @sameerz