Skip to content

Fix truncate() function #188

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.

Sign up for GitHub

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

Merged
margarit-h merged 10 commits into from
Dec 20, 2022
Merged

Fix truncate() function #188

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2ee64f3
Fix truncate() function
margarit-h Dec 9, 2022
ca87242
Address PR review feedback
margarit-h Dec 13, 2022
10ae053
Edit a javadoc
margarit-h Dec 13, 2022
a9e18e5
Add/fix tests
margarit-h Dec 13, 2022
ba76b90
Address PR review feedback
margarit-h Dec 13, 2022
d91ec92
Added more tests
margarit-h Dec 14, 2022
21619b4
Addressed more PR review feedback
margarit-h Dec 14, 2022
8599dbd
minor fix
margarit-h Dec 14, 2022
5622094
Clean up, apdate a unit test
margarit-h Dec 19, 2022
3cde7ce
minor fix
margarit-h Dec 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions core/src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/


package org.opensearch.sql.expression.operator.arthmetic;

import java.math.BigDecimal;
import java.math.RoundingMode;
import lombok.experimental.UtilityClass;

@UtilityClass

@Yury-Fridlyand Yury-Fridlyand Dec 10, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@acarbonetto acarbonetto Dec 12, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public class MathUtil {

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

@Yury-Fridlyand Yury-Fridlyand Dec 10, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

if (numberToTruncate > 0) {

@acarbonetto acarbonetto Dec 12, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

return new BigDecimal(String.valueOf(numberToTruncate))
.setScale(numberOfDecimals, RoundingMode.FLOOR);
} else {
return new BigDecimal(String
.valueOf(numberToTruncate)).setScale(numberOfDecimals, RoundingMode.CEILING);
}
}
}
12 changes: 4 additions & 8 deletions .../src/main/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())),

@acarbonetto acarbonetto Dec 12, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

LONG, INTEGER, INTEGER),
FunctionDSL.impl(
FunctionDSL.nullMissingHandling(
(x, y) -> new ExprLongValue(
new BigDecimal(x.integerValue()).setScale(y.integerValue(),
RoundingMode.DOWN).longValue())),
MathUtil.truncateNumber(x.longValue(), y.integerValue()).longValue())),
LONG, LONG, INTEGER),
FunctionDSL.impl(
FunctionDSL.nullMissingHandling(
(x, y) -> new ExprDoubleValue(
new BigDecimal(x.floatValue()).setScale(y.integerValue(),
RoundingMode.DOWN).doubleValue())),
MathUtil.truncateNumber(x.floatValue(), y.integerValue()).doubleValue())),
DOUBLE, FLOAT, INTEGER),
FunctionDSL.impl(
FunctionDSL.nullMissingHandling(
(x, y) -> new ExprDoubleValue(
new BigDecimal(x.doubleValue()).setScale(y.integerValue(),
RoundingMode.DOWN).doubleValue())),
MathUtil.truncateNumber(x.doubleValue(), y.integerValue()).doubleValue())),
DOUBLE, DOUBLE, INTEGER));
}

Expand Down
16 changes: 8 additions & 8 deletions .../test/java/org/opensearch/sql/expression/operator/arthmetic/MathematicalFunctionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1721,51 +1721,51 @@ public void sqrt_missing_value() {
* Test truncate with integer value.
*/
@ParameterizedTest(name = "truncate({0}, {1})")
@ValueSource(ints = {2, -2})
@ValueSource(ints = {2, -2, Integer.MAX_VALUE, Integer.MIN_VALUE})
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(MathUtil.truncateNumber(value, 1).longValue())));
assertEquals(String.format("truncate(%s, 1)", value), truncate.toString());
}

/**
* Test truncate with long value.
*/
@ParameterizedTest(name = "truncate({0}, {1})")
@ValueSource(longs = {2L, -2L})
@ValueSource(longs = {2L, -2L, Long.MAX_VALUE, Long.MIN_VALUE})
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(MathUtil.truncateNumber(value, 1).longValue())));
assertEquals(String.format("truncate(%s, 1)", value), truncate.toString());
}

/**
* Test truncate with float value.
*/
@ParameterizedTest(name = "truncate({0}, {1})")
@ValueSource(floats = {2F, -2F})
@ValueSource(floats = {2F, -2F, Float.MAX_VALUE, Float.MIN_VALUE})
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(MathUtil.truncateNumber(value, 1).doubleValue())));
assertEquals(String.format("truncate(%s, 1)", value), truncate.toString());
}

/**
* Test truncate with double value.
*/
@ParameterizedTest(name = "truncate({0}, {1})")
@ValueSource(doubles = {2D, -2D})
@ValueSource(doubles = {2D, -1.2D, Double.MAX_VALUE, Double.MIN_VALUE})
Comment thread
acarbonetto marked this conversation as resolved.
Outdated
public void truncate_double_value(Double 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(MathUtil.truncateNumber(value, 1).doubleValue())));
assertEquals(String.format("truncate(%s, 1)", value), truncate.toString());
}

Expand Down
8 changes: 8 additions & 0 deletions integ-test/src/test/java/org/opensearch/sql/sql/MathematicalFunctionIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,14 @@ public void testTruncate() throws IOException {
result = executeQuery("select truncate(-56, -1)");
verifySchema(result, schema("truncate(-56, -1)", null, "long"));
verifyDataRows(result, rows(-50));

result = executeQuery("select truncate(-1.2, 1)");
verifySchema(result, schema("truncate(-1.2, 1)", null, "double"));
verifyDataRows(result, rows(-1.2));

result = executeQuery("select truncate(1004.3, 1)");
verifySchema(result, schema("truncate(1004.3, 1)", null, "double"));
verifyDataRows(result, rows(1004.3));
}

@Test
Expand Down