Skip to content

Improve Math ITs & Correct MOD Function #9

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
merged 2 commits into from
Mar 19, 2025
Merged

Improve Math ITs & Correct MOD Function #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
557794b
Replace containsMessage with verifyErrorMessageContains in math ITs
yuancu Mar 18, 2025
58e42da
Correct MOD return types
yuancu Mar 19, 2025
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
19 changes: 12 additions & 7 deletions core/src/main/java/org/opensearch/sql/calcite/udf/mathUDF/ModFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.opensearch.sql.calcite.udf.mathUDF;

import java.math.BigDecimal;
import org.opensearch.sql.calcite.udf.UserDefinedFunction;

/**
Expand Down Expand Up @@ -42,18 +43,22 @@ public Object eval(Object... args) {
// It returns negative values when l0 is negative
long result = l0 % l1;
// Return the wider type between l0 and l1
if (num0 instanceof Integer && num1 instanceof Integer) {
return (int) result;
if (num0 instanceof Long || num1 instanceof Long) {
return result;
}
return result;
return (int) result;
}

double d0 = num0.doubleValue();
double d1 = num1.doubleValue();
return d0 % d1;
BigDecimal b0 = new BigDecimal(num0.toString());
BigDecimal b1 = new BigDecimal(num1.toString());
BigDecimal result = b0.remainder(b1);
if (num0 instanceof Double || num1 instanceof Double) {
return result.doubleValue();
}
return result.floatValue();
}

private boolean isIntegral(Number n) {
return n instanceof Integer || n instanceof Long;
return n instanceof Byte || n instanceof Short || n instanceof Integer || n instanceof Long;
}
}
97 changes: 57 additions & 40 deletions ...test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBuiltinFunctionIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@
*/
package org.opensearch.sql.calcite.standalone;

import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DATATYPE_NUMERIC;
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG;
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_STATE_COUNTRY;
import static org.opensearch.sql.util.MatcherUtils.closeTo;
import static org.opensearch.sql.util.MatcherUtils.rows;
import static org.opensearch.sql.util.MatcherUtils.schema;
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;
import static org.opensearch.sql.util.MatcherUtils.verifyDataRowsInOrder;
import static org.opensearch.sql.util.MatcherUtils.verifyErrorMessageContains;
import static org.opensearch.sql.util.MatcherUtils.verifySchema;

import java.io.IOException;
Expand All @@ -22,16 +25,8 @@ public void init() throws IOException {
super.init();
loadIndex(Index.STATE_COUNTRY);
loadIndex(Index.STATE_COUNTRY_WITH_NULL);
}

private static boolean containsMessage(Throwable throwable, String message) {
while (throwable != null) {
if (throwable.getMessage() != null && throwable.getMessage().contains(message)) {
return true;
}
throwable = throwable.getCause();
}
return false;
loadIndex(Index.DATA_TYPE_NUMERIC);
loadIndex(Index.DOG);
}

@Test
Expand All @@ -53,7 +48,8 @@ public void testSqrtNegativeArgShouldReturnNull() {
JSONObject actual =
executeQuery(
String.format(
"source=%s | head 1 | eval neg = sqrt(-1) | fields neg", TEST_INDEX_STATE_COUNTRY));
"source=%s | head 1 | eval neg = sqrt(-1 * age) | fields neg",
TEST_INDEX_STATE_COUNTRY));
verifyDataRows(actual, rows((Object) null));
}

Expand All @@ -65,22 +61,18 @@ public void testSqrtNanArgShouldThrowError() {
() ->
executeQuery(
String.format(
"source=%s | head 1 | eval neg = sqrt('1') | fields neg",
"source=%s | head 1 | eval sqrt_name = sqrt(name) | fields sqrt_name",
TEST_INDEX_STATE_COUNTRY)));
String errorMsg = "Invalid argument type: Expected a numeric value";
assertTrue(
String.format(
"SQRT with non-numeric arguments should throw an error that contains message: %s",
errorMsg),
containsMessage(nanException, errorMsg));
verifyErrorMessageContains(nanException, "Invalid argument type: Expected a numeric value");
}

@Test
public void testSinAndCosAndAsinAndAcos() {
JSONObject actual =
executeQuery(
String.format(
"source=%s | eval res = acos(cos(asin(sin(1)))) | head 1 | fields res",
"source=%s | where month = 4 | head 1 | eval res = acos(cos(asin(sin(month / 4))))"
+ " | head 1 | fields res",
TEST_INDEX_STATE_COUNTRY));

verifySchema(actual, schema("res", "double"));
Expand All @@ -92,8 +84,9 @@ public void testAsinAndAcosInvalidArgShouldReturnNull() {
JSONObject actual =
executeQuery(
String.format(
"source=%s | head 1 | eval s = asin(10), c = acos(-2) | fields s, c",
TEST_INDEX_STATE_COUNTRY));
"source=%s | where byte_number > 1 | head 1 | eval s = asin(byte_number), c ="
+ " acos(-1 * byte_number) | fields s, c",
TEST_INDEX_DATATYPE_NUMERIC));

verifySchema(actual, schema("s", "double"), schema("c", "double"));
verifyDataRows(actual, rows(null, null));
Expand Down Expand Up @@ -142,10 +135,16 @@ public void testConvNegateValue() {
JSONObject actual =
executeQuery(
String.format(
"source=%s | head 1 | eval hello = conv('29234652', 10, 36), negate ="
+ " conv(-29234652, 10, 36) | fields hello, negate",
TEST_INDEX_STATE_COUNTRY));
verifyDataRows(actual, rows("hello", "-hello"));
"source=%s | where dog_name = conv('1732835878', 10, 36) | eval negate ="
+ " conv('-1732835878', 10, 36), number = conv(dog_name, 36, 10) | fields"
+ " dog_name, negate, number",
TEST_INDEX_DOG));
verifySchema(
actual,
schema("dog_name", "string"),
schema("negate", "string"),
schema("number", "string"));
verifyDataRows(actual, rows("snoopy", "-snoopy", "1732835878"));
}

@Test
Expand All @@ -158,10 +157,7 @@ public void testConvWithInvalidRadix() {
String.format(
"source=%s | eval invalid = conv('0000', 1, 36) | fields invalid",
TEST_INDEX_STATE_COUNTRY)));
String errorMsg = "radix 1 less than Character.MIN_RADIX";
assertTrue(
String.format("CONV should throw an exception that contains message: %s", errorMsg),
containsMessage(invalidRadixException, errorMsg));
verifyErrorMessageContains(invalidRadixException, "radix 1 less than Character.MIN_RADIX");
}

@Test
Expand Down Expand Up @@ -245,23 +241,30 @@ public void testModFloatAndNegative() {
JSONObject actual =
executeQuery(
String.format(
"source=%s | head 1 | eval f = mod(3.1, 2), n = -3 %% 2, nf = -3.1 %% 2 | fields f,"
+ " n, nf",
TEST_INDEX_STATE_COUNTRY));
verifySchema(actual, schema("f", "double"), schema("n", "integer"), schema("nf", "double"));
verifyDataRows(actual, closeTo(1.1, -1, -1.1));
"source=%s | eval f = mod(float_number, 2), n = -1 * short_number %% 2, nd = -1 *"
+ " double_number %% 2 | fields f, n, nd",
TEST_INDEX_DATATYPE_NUMERIC));
verifySchema(actual, schema("f", "float"), schema("n", "integer"), schema("nd", "double"));
verifyDataRows(actual, closeTo(0.2, -1, -1.1));
}

@Test
public void testModShouldReturnWiderType() {
public void testModShouldReturnWiderTypes() {
JSONObject actual =
executeQuery(
String.format(
"source=%s | head 1 | eval i = mod(2147483647, 2), l = mod(2147483648, 2), "
+ "d = mod(3, 2.1) | fields i, l, d",
TEST_INDEX_STATE_COUNTRY));
verifySchema(actual, schema("i", "integer"), schema("l", "long"), schema("d", "double"));
verifyDataRows(actual, closeTo(1, 0, 0.9));
"source=%s | eval b = byte_number %% 2, i = mod(integer_number, 3), l ="
+ " mod(long_number, 2), f = float_number %% 2, d = mod(double_number, 2) |"
+ " fields b, i, l, f, d",
TEST_INDEX_DATATYPE_NUMERIC));
verifySchema(
actual,
schema("b", "integer"),
schema("i", "integer"),
schema("l", "long"),
schema("f", "float"),
schema("d", "double"));
verifyDataRows(actual, closeTo(0, 2, 1, 0.2, 1.1));
}

@Test
Expand All @@ -274,6 +277,20 @@ public void testModByZeroShouldReturnNull() {
verifyDataRows(actual, rows((Object) null));
}

@Test
public void testMod3ArgsShouldThrowIllegalArgError() {
Exception wrongArgException =
assertThrows(
IllegalArgumentException.class,
() ->
executeQuery(
String.format(
"source=%s | eval z = mod(float_number, integer_number, byte_number) |"
+ " fields z",
TEST_INDEX_DATATYPE_NUMERIC)));
verifyErrorMessageContains(wrongArgException, "MOD function requires exactly two arguments");
}

@Test
public void testRadiansAndDegrees() {
JSONObject actual =
Expand Down
Loading