Fix truncate() function #188

margarit-h · 2022-12-09T23:30:41Z

Signed-off-by: Margarit Hakobyan [email protected]

Description

Fixes truncate() returning incorrect results for some input data issue.
Example of a query result after the proposed fix:

opensearchsql> SELECT ddouble.key, ddouble.val, TRUNCATE(ddouble.val, 1) AS `EXPR$2` FROM ddouble;                                                    
fetched rows / total rows = 20/20
+----------------------------+------------------------+------------------------+
| key                        | val                    | EXPR$2                 |
|----------------------------+------------------------+------------------------|
| null                       | null                   | null                   |
| 001: min long              | -9.223372036854776e+18 | -9.223372036854776e+18 |
| 002: min int minus one     | -2147483649.0          | -2147483649.0          |
| 003: min int               | -2147483648.0          | -2147483648.0          |
| 004: min short minus one   | -32769.0               | -32769.0               |
| 005: min short             | -32768.0               | -32768.0               |
| 006: pgtest float8 value 2 | -34.84                 | -34.8                  |
| 007: -two                  | -2.0                   | -2.0                   |
| 008: -1.2                  | -1.2                   | -1.2                   |
| 009: -one                  | -1.0                   | -1.0                   |
| 010: zero                  | 0.0                    | 0.0                    |
| 011: one                   | 1.0                    | 1.0                    |
| 012: 1.3                   | 1.3                    | 1.3                    |
| 013: two                   | 2.0                    | 2.0                    |
| 014: pgtest float8 value 1 | 1004.3                 | 1004.3                 |
| 015: max short             | 32767.0                | 32767.0                |
| 016: max short plus one    | 32768.0                | 32768.0                |
| 017: max int               | 2147483647.0           | 2147483647.0           |
| 018: max int plus one      | 2147483648.0           | 2147483648.0           |
| 019: max long              | 9.223372036854776e+18  | 9.223372036854776e+18  |
+----------------------------+------------------------+------------------------+

Issues Resolved

opensearch-project#1043

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Margarit Hakobyan <[email protected]>

codecov · 2022-12-09T23:38:53Z

Codecov Report

Merging #188 (3cde7ce) into integ-fix-truncate (c923e80) will decrease coverage by 2.49%.
The diff coverage is 100.00%.

@@                   Coverage Diff                    @@
##             integ-fix-truncate     #188      +/-   ##
========================================================
- Coverage                 98.31%   95.81%   -2.50%     
- Complexity                 3521     3525       +4     
========================================================
  Files                       342      352      +10     
  Lines                      8700     9362     +662     
  Branches                    554      677     +123     
========================================================
+ Hits                       8553     8970     +417     
- Misses                      142      334     +192     
- Partials                      5       58      +53

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`98.31% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ssion/operator/arthmetic/MathematicalFunction.java	`100.00% <100.00%> (ø)`
workbench/public/components/SQLPage/SQLPage.tsx	`100.00% <0.00%> (ø)`
...h/public/components/QueryLanguageSwitch/Switch.tsx	`85.71% <0.00%> (ø)`
workbench/public/components/app.tsx	`0.00% <0.00%> (ø)`
workbench/public/components/Header/Header.tsx	`100.00% <0.00%> (ø)`
...ublic/components/QueryResults/QueryResultsBody.tsx	`68.32% <0.00%> (ø)`
workbench/public/components/PPLPage/PPLPage.tsx	`56.52% <0.00%> (ø)`
workbench/public/utils/PanelWrapper.tsx	`100.00% <0.00%> (ø)`
workbench/public/components/Main/main.tsx	`53.00% <0.00%> (ø)`
workbench/public/application.tsx	`0.00% <0.00%> (ø)`
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Yury-Fridlyand · 2022-12-10T01:12:47Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathUtil.java

+   * @param numberOfDecimals required decimal places
+   * @return truncated number as {@link BigDecimal}
+   */
+  public static BigDecimal truncateNumber(double numberToTruncate, int numberOfDecimals) {


Please, add unit tests with different numberOfDecimals values. The test should use another function to validate the result, for example, substring:

assertEquals(value.toString().subString(...), truncateNumber(value, scale).toString())

Yury-Fridlyand · 2022-12-10T01:56:28Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathUtil.java

+import java.math.RoundingMode;
+import lombok.experimental.UtilityClass;
+
+@UtilityClass


This could be troublesome: opensearch-project#1045 (comment)

I'm not sure if MathUtil is necessary. Let's include private function truncateNumber in MathematicalFunction.java.

acarbonetto · 2022-12-12T18:42:32Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathUtil.java

+   * @return truncated number as {@link BigDecimal}
+   */
+  public static BigDecimal truncateNumber(double numberToTruncate, int numberOfDecimals) {
+    if (numberToTruncate > 0) {


Better to use a conditional instead of if...else:

return new BigDecimal(String.valueOf(numberToTruncate)).setScale(numberOfDecimals, numberToTruncate > 1.0 ? RoundingMode.FLOOR : RoundingMode.CEILING);

or

RoundingMode roundingMode = numberToTruncate > 1.0 ? RoundingMode.FLOOR : RoundingMode.CEILING; BigDecimal bd = new BigDecimal(String.valueOf(numberToTruncate)).setScale(numberOfDecimals, roundingMode);

acarbonetto · 2022-12-12T18:46:24Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathUtil.java

+import java.math.RoundingMode;
+import lombok.experimental.UtilityClass;
+
+@UtilityClass


I'm not sure if MathUtil is necessary. Let's include private function truncateNumber in MathematicalFunction.java.

...src/test/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunctionTest.java

acarbonetto · 2022-12-12T18:49:59Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunction.java

@@ -500,26 +500,22 @@ private static DefaultFunctionResolver truncate() {
        FunctionDSL.impl(
            FunctionDSL.nullMissingHandling(
                (x, y) -> new ExprLongValue(
-                    new BigDecimal(x.integerValue()).setScale(y.integerValue(),
-                        RoundingMode.DOWN).longValue())),
+                        MathUtil.truncateNumber(x.integerValue(), y.integerValue()).longValue())),


this is going to convert x.integerValue() into a double since truncateNumber only accepts double. We should have a truncateNumber function for int, long, float, etc. I'm not sure this saves much effort/code?

Signed-off-by: Margarit Hakobyan <[email protected]>

acarbonetto · 2022-12-13T21:27:02Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathUtil.java

+   * @return truncated number as double
+   */
+  public static double truncateDouble(double numberToTruncate, int numberOfDecimals) {
+    return new BigDecimal(String.valueOf(numberToTruncate)).setScale(numberOfDecimals,


I'm not sure it worthwhile to have this class. These are all single-line calls used a single time. I would prefer to remove this file and include the code in the MathematicalFunction.java code.

Like:

FunctionDSL.nullMissingHandling( (x, y) -> new ExprLongValue( new BigDecimal(String.valueOf(numberToTruncate)).setScale(numberOfDecimals, numberToTruncate > 0 ? RoundingMode.FLOOR : RoundingMode.CEILING).doubleValue()), LONG, INTEGER, INTEGER),

Signed-off-by: Margarit Hakobyan <[email protected]>

Yury-Fridlyand

Try adding tests where second arg is not 1, e.g:

truncate(1.1, 2)
truncate(x, 0)
truncate(x, -1)
truncate(x, 100)
truncate(Math.PI, 4)

and so on...

Signed-off-by: Margarit Hakobyan <[email protected]>

MaxKsyunz

@margarit-h one suggestion I had is to use BigDecimal.valueOf instead of new BigDecimal(String.valueOf(...)).

Everything else looks good.

MaxKsyunz · 2022-12-14T20:58:48Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunction.java

            LONG, INTEGER, INTEGER),
        FunctionDSL.impl(
            FunctionDSL.nullMissingHandling(
                (x, y) -> new ExprLongValue(
-                    new BigDecimal(x.integerValue()).setScale(y.integerValue(),
-                        RoundingMode.DOWN).longValue())),
+                        new BigDecimal(String.valueOf(x.longValue())).setScale(y.integerValue(),


Suggested change

new BigDecimal(String.valueOf(x.longValue())).setScale(y.integerValue(),

BigDecimal.valueOf(x.longValue()).setScale(y.integerValue(),

Addressed in 21619b4

MaxKsyunz · 2022-12-14T21:04:18Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunction.java

@@ -500,26 +500,30 @@ private static DefaultFunctionResolver truncate() {
        FunctionDSL.impl(
            FunctionDSL.nullMissingHandling(
                (x, y) -> new ExprLongValue(
-                    new BigDecimal(x.integerValue()).setScale(y.integerValue(),
-                        RoundingMode.DOWN).longValue())),
+                        new BigDecimal(String.valueOf(x.integerValue())).setScale(y.integerValue(),


Suggested change

new BigDecimal(String.valueOf(x.integerValue())).setScale(y.integerValue(),

BigDecimal.valueOf(x.integerValue()).setScale(y.integerValue(),

Addressed in 21619b4

MaxKsyunz · 2022-12-14T21:04:28Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunction.java

            LONG, LONG, INTEGER),
        FunctionDSL.impl(
            FunctionDSL.nullMissingHandling(
                (x, y) -> new ExprDoubleValue(
-                    new BigDecimal(x.floatValue()).setScale(y.integerValue(),
-                        RoundingMode.DOWN).doubleValue())),
+                        new BigDecimal(String.valueOf(x.floatValue())).setScale(y.integerValue(),


Suggested change

new BigDecimal(String.valueOf(x.floatValue())).setScale(y.integerValue(),

BigDecimal.valueOf(x.floatValue()).setScale(y.integerValue(),

Addressed in 21619b4

MaxKsyunz · 2022-12-14T21:04:40Z

core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunction.java

            DOUBLE, FLOAT, INTEGER),
        FunctionDSL.impl(
            FunctionDSL.nullMissingHandling(
                (x, y) -> new ExprDoubleValue(
-                    new BigDecimal(x.doubleValue()).setScale(y.integerValue(),
-                        RoundingMode.DOWN).doubleValue())),
+                        new BigDecimal(String.valueOf(x.doubleValue())).setScale(y.integerValue(),


Suggested change

new BigDecimal(String.valueOf(x.doubleValue())).setScale(y.integerValue(),

new BigDecimal.valueOf(x.doubleValue()).setScale(y.integerValue(),

Addressed in 21619b4

MaxKsyunz · 2022-12-14T21:05:27Z

...src/test/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunctionTest.java

  public void truncate_int_value(Integer value) {
    FunctionExpression truncate = DSL.truncate(DSL.literal(value), DSL.literal(1));
    assertThat(
        truncate.valueOf(valueEnv()), allOf(hasType(LONG),
-            hasValue(new BigDecimal(value).setScale(1, RoundingMode.DOWN).longValue())));
+              hasValue(new BigDecimal(String.valueOf(value)).setScale(1,


Suggested change

hasValue(new BigDecimal(String.valueOf(value)).setScale(1,

hasValue(BigDecimal.valueOf(value).setScale(1,

Addressed in 21619b4

MaxKsyunz · 2022-12-14T21:05:43Z

...src/test/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunctionTest.java

  public void truncate_long_value(Long value) {
    FunctionExpression truncate = DSL.truncate(DSL.literal(value), DSL.literal(1));
    assertThat(
        truncate.valueOf(valueEnv()), allOf(hasType(LONG),
-            hasValue(new BigDecimal(value).setScale(1, RoundingMode.DOWN).longValue())));
+            hasValue(new BigDecimal(String.valueOf(value)).setScale(1,


Suggested change

hasValue(new BigDecimal(String.valueOf(value)).setScale(1,

hasValue(BigDecimal.valueOf(value).setScale(1,

Addressed in 21619b4

MaxKsyunz · 2022-12-14T21:06:39Z

...src/test/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunctionTest.java

  public void truncate_float_value(Float value) {
    FunctionExpression truncate = DSL.truncate(DSL.literal(value), DSL.literal(1));
    assertThat(
        truncate.valueOf(valueEnv()), allOf(hasType(DOUBLE),
-            hasValue(new BigDecimal(value).setScale(1, RoundingMode.DOWN).doubleValue())));
+            hasValue(new BigDecimal(String.valueOf(value)).setScale(1,


Suggested change

hasValue(new BigDecimal(String.valueOf(value)).setScale(1,

hasValue(BigDecimal.valueOf(value).setScale(1,

Addressed in 21619b4

integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java

...src/test/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunctionTest.java

Signed-off-by: Margarit Hakobyan <[email protected]>

* Fix truncate() function (#188) Signed-off-by: Margarit Hakobyan <[email protected]> Signed-off-by: Margarit Hakobyan <[email protected]>

…#1213) * Fix truncate() function (#188) Signed-off-by: Margarit Hakobyan <[email protected]> Signed-off-by: Margarit Hakobyan <[email protected]> (cherry picked from commit 7714819) Co-authored-by: Margarit Hakobyan <[email protected]>

Fix truncate() function

2ee64f3

Signed-off-by: Margarit Hakobyan <[email protected]>

margarit-h changed the title ~~Fix truncate() function~~ [WIP:]Fix truncate() function Dec 9, 2022

margarit-h requested a review from a team December 9, 2022 23:31

Yury-Fridlyand reviewed Dec 10, 2022

View reviewed changes

acarbonetto requested changes Dec 12, 2022

View reviewed changes

margarit-h added 2 commits December 12, 2022 16:13

Address PR review feedback

ca87242

Signed-off-by: Margarit Hakobyan <[email protected]>

Edit a javadoc

10ae053

Signed-off-by: Margarit Hakobyan <[email protected]>

margarit-h changed the title ~~[WIP:]Fix truncate() function~~ Fix truncate() function Dec 13, 2022

Yury-Fridlyand approved these changes Dec 13, 2022

View reviewed changes

Add/fix tests

a9e18e5

Signed-off-by: Margarit Hakobyan <[email protected]>

acarbonetto reviewed Dec 13, 2022

View reviewed changes

Address PR review feedback

ba76b90

Signed-off-by: Margarit Hakobyan <[email protected]>

Yury-Fridlyand requested review from acarbonetto, forestmvey, GumpacG, MaxKsyunz, MitchellGale and GabeFernandez310 December 14, 2022 01:47

Yury-Fridlyand approved these changes Dec 14, 2022

View reviewed changes

Added more tests

d91ec92

Signed-off-by: Margarit Hakobyan <[email protected]>

MaxKsyunz reviewed Dec 14, 2022

View reviewed changes

margarit-h added 2 commits December 14, 2022 15:34

Addressed more PR review feedback

21619b4

Signed-off-by: Margarit Hakobyan <[email protected]>

minor fix

8599dbd

Signed-off-by: Margarit Hakobyan <[email protected]>

acarbonetto approved these changes Dec 19, 2022

View reviewed changes

MaxKsyunz approved these changes Dec 19, 2022

View reviewed changes

margarit-h added 2 commits December 19, 2022 11:36

Clean up, apdate a unit test

5622094

Signed-off-by: Margarit Hakobyan <[email protected]>

minor fix

3cde7ce

Signed-off-by: Margarit Hakobyan <[email protected]>

margarit-h merged commit 2fea6e6 into integ-fix-truncate Dec 20, 2022

Yury-Fridlyand pushed a commit that referenced this pull request Jan 4, 2023

Fix truncate() function (opensearch-project#1197)

7714819

* Fix truncate() function (#188) Signed-off-by: Margarit Hakobyan <[email protected]> Signed-off-by: Margarit Hakobyan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix truncate() function #188

Fix truncate() function #188

margarit-h commented Dec 9, 2022 •

edited

Loading

codecov bot commented Dec 9, 2022 •

edited

Loading

Yury-Fridlyand Dec 10, 2022

Yury-Fridlyand Dec 10, 2022

acarbonetto Dec 12, 2022

acarbonetto Dec 12, 2022

acarbonetto Dec 12, 2022

acarbonetto Dec 12, 2022

acarbonetto Dec 13, 2022

Yury-Fridlyand left a comment

MaxKsyunz left a comment

MaxKsyunz Dec 14, 2022

margarit-h Dec 14, 2022

MaxKsyunz Dec 14, 2022

margarit-h Dec 14, 2022

MaxKsyunz Dec 14, 2022

margarit-h Dec 14, 2022

MaxKsyunz Dec 14, 2022

margarit-h Dec 14, 2022

MaxKsyunz Dec 14, 2022

margarit-h Dec 14, 2022

MaxKsyunz Dec 14, 2022

margarit-h Dec 14, 2022

MaxKsyunz Dec 14, 2022

margarit-h Dec 14, 2022

	new BigDecimal(String.valueOf(x.longValue())).setScale(y.integerValue(),
	BigDecimal.valueOf(x.longValue()).setScale(y.integerValue(),

	new BigDecimal(String.valueOf(x.integerValue())).setScale(y.integerValue(),
	BigDecimal.valueOf(x.integerValue()).setScale(y.integerValue(),

	new BigDecimal(String.valueOf(x.floatValue())).setScale(y.integerValue(),
	BigDecimal.valueOf(x.floatValue()).setScale(y.integerValue(),

	new BigDecimal(String.valueOf(x.doubleValue())).setScale(y.integerValue(),
	new BigDecimal.valueOf(x.doubleValue()).setScale(y.integerValue(),

	hasValue(new BigDecimal(String.valueOf(value)).setScale(1,
	hasValue(BigDecimal.valueOf(value).setScale(1,

Fix truncate() function #188

Fix truncate() function #188

Conversation

margarit-h commented Dec 9, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Dec 9, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

MaxKsyunz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

margarit-h commented Dec 9, 2022 •

edited

Loading

codecov bot commented Dec 9, 2022 •

edited

Loading