Add math udf #3390

xinyual · 2025-03-06T11:48:54Z

Description

This PR add math built-in functions.
And also add corresponding IT for them.
Due to the lack of IT in spark PPL, we also implement some by ourselves.

Functions	Covered by Spark IT	Covered by IT
abs	Y	Y
acos		Y
asin		Y
atan		Y
atan2		Y
cbrt		Y
ceiling	Y	Y
conv		Y
cos		Y
cot		Y
crc32		Y
degrees		Y
e		Y
exp		Y
floor	Y	Y (Modified)
ln	Y	Y
log		Y
log2		Y
log10		Y
mod	Y	Y
pi		Y
pow	Y	Y
radians		Y
rand		Y
round		Y
sign		Y
sin		Y
sqrt	Y	Y

The integration tests covered by Spark are copied to this IT (Apart from floor). The rest is implemented by ourselves.
./gradlew :integ-test:integTest --tests 'CalciteIT' succeed in local

Related Issues

Resolves #3398

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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: xinyual <xinyual@amazon.com>

- Additionally implement user-defined ConvFunction Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Add math udf

Signed-off-by: xinyual <xinyual@amazon.com>

LantaoJin

And rename UserDefineFunctionUtils to UserDefinedFunctionUtils to unify the naming style

LantaoJin · 2025-03-07T01:55:15Z

core/src/main/java/org/opensearch/sql/calcite/utils/BuiltinFunctionUtils.java

+import org.apache.calcite.sql.type.SqlTypeName;
 import org.opensearch.sql.calcite.CalcitePlanContext;
-import org.opensearch.sql.calcite.udf.mathUDF.SqrtFunction;
+import org.opensearch.sql.calcite.udf.mathUDF.*;


LantaoJin · 2025-03-07T01:59:30Z

core/src/main/java/org/opensearch/sql/calcite/udf/mathUDF/ConvFunction.java

+    // Convert to the target base; BigInteger's toString(...) yields lowercase above 9
+    String result = value.toString(toBase);
+
+    // Convert to uppercase to align with MySQL's behavior


why align with MySQL's behavior here? Have you checked the exists behavior in v2?

In V2, it returns everything in lowercase. I'll fix it.

LantaoJin · 2025-03-07T02:14:14Z

core/src/main/java/org/opensearch/sql/calcite/utils/BuiltinFunctionUtils.java

+      case "LOG10":
+        return SqlStdOperatorTable.LOG10;
+      case "MOD":
+        return TransferUserDefinedFunction(ModFunction.class, "MOD", ReturnTypes.DOUBLE);


What's the difference between this and https://github.com/apache/calcite/blob/e5e7faeff5985bc1b2342144b2bd31ca8ea84d3a/core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java#L1719?

The MOD in OpenSearch supports MOD(DOUBLE, LONG), e.g. MOD(3.1, 2) = 1.1.

However, the MOD defined in Calcite supports only long parameters.

Comment the explanation in code

LantaoJin · 2025-03-07T02:18:19Z

core/src/main/java/org/opensearch/sql/calcite/utils/BuiltinFunctionUtils.java

+      case "CEILING":
+        return SqlStdOperatorTable.CEIL;
+      case "CONV":
+        return TransferUserDefinedFunction(ConvFunction.class, "CONVERT", ReturnTypes.BIGINT);


What's the difference between this and https://github.com/apache/calcite/blob/e5e7faeff5985bc1b2342144b2bd31ca8ea84d3a/core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java#L1636

They are defined differently. According to its implementation (definition & underlying implementation), the CONVERT in MySQL is to convert between charsets like UTF-8 and LATIN1, whereas the CONV in OpenSearch is to convert between different base, e.g. binary to hex.

penghuo · 2025-03-07T04:53:50Z

ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLMathFunctionTest.java

+    String expectedLogical =
+        "LogicalProject(ASIN=[$8])\n"
+            + "  LogicalSort(fetch=[2])\n"
+            + "    LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4],"
+            + " SAL=[$5], COMM=[$6], DEPTNO=[$7], ASIN=[ASIN(0)])\n"
+            + "      LogicalTableScan(table=[[scott, EMP]])\n";


We should verify the precise content rather than the entire query plan. For example, if LogicalSort does not appear in the query plan, all irrelevant unit tests must be updated accordingly.

yes, @yuancu please simply the UTs and complicate the ITs

penghuo · 2025-03-07T04:55:44Z

core/src/main/java/org/opensearch/sql/calcite/udf/mathUDF/ModFunction.java

+  @Override
+  public Object eval(Object... args) {
+    if (args.length < 2) {
+      throw new IllegalArgumentException("At least two arguments are required");


make the error message more precise. "Mod function required two arguments, but it has args.length"

penghuo · 2025-03-07T04:57:31Z

core/src/main/java/org/opensearch/sql/calcite/udf/mathUDF/ModFunction.java

+
+      return num0 % num1; // Handles both float and double cases
+    } else {
+      throw new IllegalArgumentException("Invalid argument types: Expected numeric values");


make the error message more precise.

penghuo · 2025-03-07T04:58:34Z

core/src/main/java/org/opensearch/sql/calcite/udf/mathUDF/ModFunction.java

+        throw new ArithmeticException("Modulo by zero is not allowed");
+      }
+
+      return num0 % num1; // Handles both float and double cases


comments from LLM? if so, remove it.

Is above "Modulo" a typo?

penghuo · 2025-03-07T04:58:50Z

core/src/main/java/org/opensearch/sql/calcite/udf/mathUDF/ModFunction.java

+
+import org.opensearch.sql.calcite.udf.UserDefinedFunction;
+
+public class ModFunction implements UserDefinedFunction {


consider add UT?

penghuo · 2025-03-07T05:00:00Z

core/src/main/java/org/opensearch/sql/calcite/udf/mathUDF/ConvFunction.java

+public class ConvFunction implements UserDefinedFunction {
+  @Override
+  public Object eval(Object... args) {
+    Object number = args[0];


why not check args.length in ConvFunction? and check args.length in ModFunction.

And need some UT or IT for edge case

…uble.NaN and Float.NaN to null Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Handle Edge Cases & Update UT/ITs for Math Functions

yuancu · 2025-03-11T03:35:52Z

I found following problems during testing:

MOD function handles negative values inconsistently in V2
MOD(-3, 2) = 1, while MOD(-3.1, 2) = -1.1.
In contrast, -3 % 2 = -1, -3.1 % 2 = -1.1 in Java; -3 % 2 = 1, -3.1 % 2 = 0.9 in Python.
The submitted implementation in this PR always returns positive values as that in Python.
ROUND is documented to return different types based on its arguments. But in V2 it always return double-typed values (which makes more sense IMO). Maybe we should consider update/correct the document?

penghuo · 2025-03-13T22:39:04Z

MOD function handles negative values inconsistently in V2
MOD(-3, 2) = 1, while MOD(-3.1, 2) = -1.1.
In contrast, -3 % 2 = -1, -3.1 % 2 = -1.1 in Java; -3 % 2 = 1, -3.1 % 2 = 0.9 in Python.
The submitted implementation in this PR always returns positive values as that in Python.

Java result make sense to me, also it align with Spark-SQL.

spark-sql (default)> select MOD(-3, 2), MOD(-3.1, 2);
mod(-3, 2)	mod(-3.1, 2)
-1	-1.1

LantaoJin · 2025-03-14T16:37:09Z

core/src/main/java/org/opensearch/sql/calcite/utils/BuiltinFunctionUtils.java

+        return SqlLibraryOperators.LOG2;
+      case "LOG10":
+        return SqlStdOperatorTable.LOG10;
+      case "MOD":


change to

case "MOD": case "%": ....

LantaoJin · 2025-03-17T04:21:30Z

@xinyual could you resolve the conflicts again?

Signed-off-by: xinyual <xinyual@amazon.com>

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

- remove comparision between string and integers - correct thrown error types Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

- add alias % to MOD - return negative when the dividend is negative Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Correct Math UDF Integration Tests

xinyual · 2025-03-17T08:15:12Z

@xinyual could you resolve the conflicts again?

Already resolve them.

LantaoJin · 2025-03-18T08:02:35Z

@yuancu spotlessJavaCheck FAILED

LantaoJin · 2025-03-18T08:07:59Z

integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBuiltinFunctionIT.java

    loadIndex(Index.STATE_COUNTRY_WITH_NULL);
  }

+  private static boolean containsMessage(Throwable throwable, String message) {


minor: use verifyErrorMessageContains

One problem in this IT is all the test cases are applying on literals instead of fields.
Could you add some IT for fields with

loadIndex(Index.DATA_TYPE_NUMERIC); loadIndex(Index.DATA_TYPE_NONNUMERIC);

Signed-off-by: xinyual <xinyual@amazon.com>

xinyual · 2025-03-18T08:22:32Z

@yuancu spotlessJavaCheck FAILED

Fixed now.

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

- additionally enrich math ITs with fields calculations Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Improve Math ITs & Correct MOD Function

LantaoJin · 2025-03-19T10:05:00Z

Fix this please

CalcitePPLMathFunctionTest > testAbsWithOverriding FAILED
    java.lang.AssertionError: 
    Expected: is "LogicalProject(SAL0=[ABS(-30)])\n  LogicalTableScan(table=[[scott, EMP]])\n"
         but: was "LogicalProject(SAL=[ABS(-30)])\n  LogicalTableScan(table=[[scott, EMP]])\n"

Signed-off-by: xinyual <xinyual@amazon.com>

LantaoJin · 2025-03-20T02:56:16Z

LGTM, approved. Merging to dev branch.

* add math udfs Signed-off-by: xinyual <xinyual@amazon.com> * add log argument Signed-off-by: xinyual <xinyual@amazon.com> * Add math function unit tests - Additionally implement user-defined ConvFunction Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Add integration tests for Calcite math functions Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Rename CalcitePPLMathFunctionsIT to CalcitePPLBuiltinFunctionIT Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * add license Signed-off-by: xinyual <xinyual@amazon.com> * apply spot Signed-off-by: xinyual <xinyual@amazon.com> * Update the implementation of CONV function to align with v2's behavior - Rename UserDefineFunctionUtils to UserDefinedFunctionUtils Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Improve code style: - enforce uniform parameter number check - comment on differences from calcite's implementation if necessary Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Simplify Calcite PPL math function unit tests Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Alter MOD and SQRT UDF to conform to documented behaviors - return null with invalid (zero, negative) arguments - return wider type for mod Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Complicate math integration tests - edge cases for UDF - combine operations or clauses Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Handle NULL return in ASIN, ACOS, SQRT and POW by convert returned Double.NaN and Float.NaN to null Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Apply spotless on math UDFs and their tests Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Remove unnecessary Double cast in SQRT UDF Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Convert returned Double.NaN and Float.NaN from math UDFs to LITERAL_NULL Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Correct math UDF integration tests - remove comparision between string and integers - correct thrown error types Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Update MOD UDF - add alias % to MOD - return negative when the dividend is negative Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Modify substring ITs Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * apply spot Signed-off-by: xinyual <xinyual@amazon.com> * Replace containsMessage with verifyErrorMessageContains in math ITs Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Correct MOD return types - additionally enrich math ITs with fields calculations Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * fix UT Signed-off-by: xinyual <xinyual@amazon.com> --------- Signed-off-by: xinyual <xinyual@amazon.com> Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> Co-authored-by: xinyual <xinyual@amazon.com> Co-authored-by: Yuanchun Shen <yuanchu@amazon.com> Signed-off-by: xinyual <xinyual@amazon.com>

xinyual and others added 6 commits March 5, 2025 10:29

add math udfs

08837b2

Signed-off-by: xinyual <xinyual@amazon.com>

add log argument

404f43d

Signed-off-by: xinyual <xinyual@amazon.com>

Add math function unit tests

17976ef

- Additionally implement user-defined ConvFunction Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Add integration tests for Calcite math functions

f04ca61

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Rename CalcitePPLMathFunctionsIT to CalcitePPLBuiltinFunctionIT

ccbfefc

Merge pull request #2 from yuancu/addMathUDF

66859e4

Add math udf

xinyual requested review from GumpacG, LantaoJin, MaxKsyunz, Swiddis, YANG-DB, Yury-Fridlyand, acarbonetto, anirudha, dai-chen, derek-ho, forestmvey, joshuali925, kavithacm, mengweieric, noCharger, penghuo, ps48, seankao-az and ykmr1224 as code owners March 6, 2025 11:48

xinyual added 2 commits March 6, 2025 19:49

add license

d059299

Signed-off-by: xinyual <xinyual@amazon.com>

apply spot

e644401

Signed-off-by: xinyual <xinyual@amazon.com>

LantaoJin added the calcite calcite migration releated label Mar 7, 2025

LantaoJin reviewed Mar 7, 2025

View reviewed changes

penghuo reviewed Mar 7, 2025

View reviewed changes

yuancu and others added 4 commits March 11, 2025 09:58

Handle NULL return in ASIN, ACOS, SQRT and POW by convert returned Do…

c7e32b4

…uble.NaN and Float.NaN to null Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Apply spotless on math UDFs and their tests

8728534

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Remove unnecessary Double cast in SQRT UDF

8f56ced

Merge pull request #3 from yuancu/addMathUDF

0515377

Handle Edge Cases & Update UT/ITs for Math Functions

LantaoJin reviewed Mar 14, 2025

View reviewed changes

xinyual and others added 6 commits March 17, 2025 14:23

merge from origin

0e7fe63

Signed-off-by: xinyual <xinyual@amazon.com>

Convert returned Double.NaN and Float.NaN from math UDFs to LITERAL_NULL

eb3e669

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Correct math UDF integration tests

7dae0f7

- remove comparision between string and integers - correct thrown error types Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Update MOD UDF

584ca60

- add alias % to MOD - return negative when the dividend is negative Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Modify substring ITs

efc1ab8

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Merge pull request #6 from yuancu/addMathUDF

6eb74d1

Correct Math UDF Integration Tests

LantaoJin reviewed Mar 18, 2025

View reviewed changes

apply spot

bb77593

Signed-off-by: xinyual <xinyual@amazon.com>

yuancu and others added 3 commits March 18, 2025 19:18

Replace containsMessage with verifyErrorMessageContains in math ITs

557794b

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Correct MOD return types

58e42da

- additionally enrich math ITs with fields calculations Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Merge pull request #9 from yuancu/addMathUDF

2a16176

Improve Math ITs & Correct MOD Function

fix UT

7b619e5

Signed-off-by: xinyual <xinyual@amazon.com>

LantaoJin merged commit 7f8cbfe into opensearch-project:feature/calcite-engine Mar 20, 2025
18 of 20 checks passed

yuancu mentioned this pull request Apr 3, 2025

Fix return types of MOD and DIVIDE UDFs #3513

Merged

6 tasks

LantaoJin mentioned this pull request Apr 11, 2025

[BUG] Some math functions return types are different between V2 and V3 #3479

Closed

LantaoJin mentioned this pull request Aug 15, 2025

[GitHub Request] Add yuancu as maintainer of OpenSearch SQL opensearch-project/.github#374

Closed


		import org.opensearch.sql.calcite.udf.UserDefinedFunction;

		public class ModFunction implements UserDefinedFunction {

Uh oh!

Add math udf #3390

Add math udf #3390

Uh oh!

Conversation

xinyual commented Mar 6, 2025 • edited by LantaoJin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

LantaoJin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuancu Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuancu Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuancu commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

penghuo commented Mar 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin commented Mar 17, 2025

Uh oh!

xinyual commented Mar 17, 2025

Uh oh!

LantaoJin commented Mar 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xinyual commented Mar 18, 2025

Uh oh!

LantaoJin commented Mar 19, 2025

Uh oh!

Uh oh!

LantaoJin commented Mar 20, 2025

Uh oh!

Reviewers

Assignees

xinyual commented Mar 6, 2025 •

edited by LantaoJin

Loading

yuancu Mar 7, 2025 •

edited

Loading

yuancu Mar 7, 2025 •

edited

Loading

yuancu commented Mar 11, 2025 •

edited

Loading