zabetak commented on code in PR #3756:
URL: https://github.com/apache/hive/pull/3756#discussion_r1027776735
##########
ql/src/test/queries/clientpositive/udf_trunc_number.q:
##########
@@ -71,4 +71,84 @@ DROP TABLE sampletable2;
DROP TABLE sampletable3;
-DROP TABLE sampletable4;
\ No newline at end of file
+DROP TABLE sampletable4;
+
+select trunc(CAST('123456789.123456789' AS DECIMAL(18,9)), 8);
Review Comment:
This test seems to pass with or without the changes in this PR. Can you
point which tests exactly fail due to overflow?
##########
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFTrunc.java:
##########
@@ -477,21 +477,38 @@ protected DoubleWritable trunc(DoubleWritable input, int
scale) {
}
protected BigDecimal trunc(BigDecimal input, int scale) {
Review Comment:
The calculations below are somewhat complex and resemble a bit what is
happening inside `BigDecimal#setScale`. Can we replace some of the logic below
with calls to `BigDecimal#setScale`?
##########
ql/src/test/queries/clientpositive/udf_trunc_number.q:
##########
@@ -71,4 +71,84 @@ DROP TABLE sampletable2;
DROP TABLE sampletable3;
-DROP TABLE sampletable4;
\ No newline at end of file
+DROP TABLE sampletable4;
Review Comment:
Wouldn't be more appropriate to add unit tests (similar to
`TestGenericUDFTrunc`) instead of qtests? The changes in this PR touch a very
specific part of the code so I am not sure we need to add end-to-end tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
