Fix #14693: support TRUNCATE function for DOUBLE and REAL #14783

fgwang7w · 2020-07-06T06:28:26Z

== RELEASE NOTES ==

General Changes

The solution is to fix issue #14693 by adding an enhancement to TRUNCATE function for allowing DOUBLE and REAL types.
Following examples show how user may use this feature:

truncate(DOUBLE '1234.56', 1) -> The outcome of the evaluation is 1234.5

truncate(REAL '-12.333', -1) -> The outcome of the evaluation is -10.0

NOTE: Neither does this patch change any behaviors of current mathemtical functionalities, nor is the existing workload being impacted. In addition, there is zero impact to data/workload migration.

presto-main/src/test/java/com/facebook/presto/operator/scalar/TestMathFunctions.java

tdcmeehan · 2020-07-09T23:34:58Z

presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java

+                decimals = NUMBER_LENGTH;
+            }
+        }
+        return floatToRawIntBits(new BigDecimal(Float.toString(numBitsToFloats)).setScale((int) decimals, BigDecimal.ROUND_DOWN).floatValue());


Suggested change

return floatToRawIntBits(new BigDecimal(Float.toString(numBitsToFloats)).setScale((int) decimals, BigDecimal.ROUND_DOWN).floatValue());

return floatToRawIntBits(new BigDecimal(numBitsToFloats).setScale((int) decimals, BigDecimal.ROUND_DOWN).floatValue());

Any reason why this had to be converted to a string? The test cases pass with the above.

BigDecimal takes type of DOUBLE not FLOAT so use String as a safe cast value to return.

presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java

presto-main/src/test/java/com/facebook/presto/operator/scalar/TestMathFunctions.java

presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java

tdcmeehan

We'll also want to add documentation for it here.

kaikalur · 2020-07-14T18:59:54Z

The one with -1 as the second param are very confusing! Is it similar to any other system like Oracle or Mysql?

fgwang7w · 2020-07-16T01:15:34Z

We'll also want to add documentation for it here.

sure will add spec doc accordingly

fgwang7w · 2020-07-16T01:16:08Z

The one with -1 as the second param are very confusing! Is it similar to any other system like Oracle or Mysql?

Yes this is commonly used in the industry, Oracle has a spec here using -1: https://docs.oracle.com/cd/B19306_01/server.102/b14200/functions200.htm

fgwang7w · 2020-07-21T06:59:09Z

We'll also want to add documentation for it here.

@tdcmeehan Thanks for the comment, I have added the doc accordingly for this function. Please help give another review. many thanks!

tdcmeehan

LGTM. @kaikalur @rongrong do you want to look?

fgwang7w · 2020-07-29T06:33:38Z

LGTM. @kaikalur @rongrong do you want to look?

Thanks @tdcmeehan !
@kaikalur @rongrong could you please help review this commit? Thanks!

rongrong · 2020-07-29T21:26:08Z

presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java

@@ -327,6 +331,51 @@ public static long truncate(@SqlType(StandardTypes.REAL) long num)
        return floatToRawIntBits((float) (Math.signum(numInFloat) * Math.floor(Math.abs(numInFloat))));
    }

+    @Description("truncate to double by dropping digits after decimal point")


If there's a limit for decimal point, I think we should be explicit about it and mention in description and in documentation. We should probably also fail the query if an out-of-range number is given, rather than silently changing the behavior.

Though if the limit is actually limitation of double then this behavior is fine. But in that case I think we also don't have to special handle the max?

Thanks for the review. Are you suggesting to document the max number of precision for double type in alignment with MAX_DECIMAL_PLACES value? Yes the limit only handles the double/real type behavior. If the max limit reaches/exceeded, the outcome should be either 0.0 or negative value. I have 2 tests covered that:

assertFunction("truncate(DOUBLE '" + Double.MAX_VALUE + "', -408)", DOUBLE, 0.0); assertFunction("truncate(DOUBLE '" + -Double.MAX_VALUE / 10 + "', 408)", DOUBLE, -Double.MAX_VALUE / 10);

Is this what you were thinking to cover? @rongrong

I mean add this to the rst file. Maybe also add it to the function description. So people know the maximum value for decimals should be within 400. Also document the behavior if the provided decimal is larger than 400. Why do we choose 400 as the max digit though? What's the logic behind this?

Actually I double-checked the oracle doc which indicates that the scale of the decimal points ranges from -84 to 127. I will change 400 to 127 to align with oracle's behavior and document it in rst file.

kaikalur

LGTM

fgwang7w · 2020-08-17T23:08:05Z

can someone please help to merge the code or does it need another review?

fgwang7w · 2020-08-19T06:14:46Z

@rongrong assign this pr to you since you left the last comment, if all looks good please help coordinate to merge. many thanks!

fgwang7w requested review from shixuan-fan, rongrong and aweisberg July 6, 2020 06:29

tdcmeehan requested a review from kaikalur July 9, 2020 23:28

tdcmeehan reviewed Jul 9, 2020

View reviewed changes

fgwang7w force-pushed the 14693 branch from 7ea31d8 to 70e0638 Compare July 11, 2020 21:26

fgwang7w requested review from tdcmeehan and removed request for rongrong and kaikalur July 11, 2020 23:16

fgwang7w force-pushed the 14693 branch from 70e0638 to 1219318 Compare July 12, 2020 01:46

tdcmeehan requested review from kaikalur and rongrong July 14, 2020 16:56

tdcmeehan requested changes Jul 14, 2020

View reviewed changes

fgwang7w force-pushed the 14693 branch from 1219318 to d9d578c Compare July 21, 2020 06:57

fgwang7w requested review from tdcmeehan and removed request for kaikalur and rongrong July 21, 2020 06:59

tdcmeehan approved these changes Jul 22, 2020

View reviewed changes

fgwang7w requested review from kaikalur and rongrong and removed request for shixuan-fan and aweisberg July 29, 2020 06:33

rongrong reviewed Jul 29, 2020

View reviewed changes

kaikalur approved these changes Jul 29, 2020

View reviewed changes

fgwang7w requested a review from rongrong August 12, 2020 06:22

fgwang7w assigned rongrong Aug 19, 2020

fgwang7w force-pushed the 14693 branch 3 times, most recently from 4685a6c to 900b104 Compare August 20, 2020 02:47

Add support for TRUNCATE function for DOUBLE and REAL

ae4dd65

fgwang7w force-pushed the 14693 branch from 900b104 to ae4dd65 Compare August 20, 2020 02:57

rongrong approved these changes Oct 13, 2020

View reviewed changes

rongrong merged commit f2965f6 into prestodb:master Oct 13, 2020

caithagoras mentioned this pull request Oct 19, 2020

Add release notes for 0.243 #15334

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #14693: support TRUNCATE function for DOUBLE and REAL #14783

Fix #14693: support TRUNCATE function for DOUBLE and REAL #14783

fgwang7w commented Jul 6, 2020

tdcmeehan Jul 9, 2020

fgwang7w Jul 11, 2020

tdcmeehan left a comment

kaikalur commented Jul 14, 2020

fgwang7w commented Jul 16, 2020

fgwang7w commented Jul 16, 2020

fgwang7w commented Jul 21, 2020

tdcmeehan left a comment

fgwang7w commented Jul 29, 2020

rongrong Jul 29, 2020

rongrong Jul 29, 2020

fgwang7w Jul 30, 2020

rongrong Aug 19, 2020

fgwang7w Aug 20, 2020

kaikalur left a comment

fgwang7w commented Aug 17, 2020

fgwang7w commented Aug 19, 2020

	return floatToRawIntBits(new BigDecimal(Float.toString(numBitsToFloats)).setScale((int) decimals, BigDecimal.ROUND_DOWN).floatValue());
	return floatToRawIntBits(new BigDecimal(numBitsToFloats).setScale((int) decimals, BigDecimal.ROUND_DOWN).floatValue());

Fix #14693: support TRUNCATE function for DOUBLE and REAL #14783

Fix #14693: support TRUNCATE function for DOUBLE and REAL #14783

Conversation

fgwang7w commented Jul 6, 2020

tdcmeehan Jul 9, 2020

Choose a reason for hiding this comment

fgwang7w Jul 11, 2020

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

kaikalur commented Jul 14, 2020

fgwang7w commented Jul 16, 2020

fgwang7w commented Jul 16, 2020

fgwang7w commented Jul 21, 2020

tdcmeehan left a comment

Choose a reason for hiding this comment

fgwang7w commented Jul 29, 2020

rongrong Jul 29, 2020

Choose a reason for hiding this comment

rongrong Jul 29, 2020

Choose a reason for hiding this comment

fgwang7w Jul 30, 2020

Choose a reason for hiding this comment

rongrong Aug 19, 2020

Choose a reason for hiding this comment

fgwang7w Aug 20, 2020

Choose a reason for hiding this comment

kaikalur left a comment

Choose a reason for hiding this comment

fgwang7w commented Aug 17, 2020

fgwang7w commented Aug 19, 2020