Add udf interface by xinyual · Pull Request #3374 · opensearch-project/sql

xinyual · 2025-03-04T07:47:27Z

Description

This pr is main:

Add UDF/UDAF interface
add some examples:
a. Directly map to calcite built-in function, but need modification for argument (optional). e.g. Atan/Atan2/pow
b. cannot find suitable built-in function, need to write our own logic. E.g. sqrt function, take aggregation function
add UT/IT for them.
Fix join data table from here

./gradlew :integ-test:integTest --tests 'CalciteIT' succeed locally

Related Issues

Resolves #[Issue number to be closed when this PR is merged]
#3310

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>

LantaoJin · 2025-03-04T07:53:57Z

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

    }
  }
+
+  static List<RexNode> translateArgument(


Since this is a framework work, we will change this method frequently, could you add some comments to explain this method for developers

Sure, already added.

LantaoJin · 2025-03-04T07:58:34Z

core/src/main/java/org/opensearch/sql/calcite/udf/Accumulator.java

+package org.opensearch.sql.calcite.udf;
+
+public interface Accumulator {
+  Object result();


some comments for this method.

could it be moved to class UserDefinedAggFunction.java since it is only used for UDAF

how about name to value()

as a accumulator, do we need another method merge() to merge two Acc objects.

Already done.

Already done.

Already done.

Can you elaborate more in which scenario we may need to do so? Accumulator is something like AggregationState, do we need to consider merge here?

4

The merge method in an Accumulator is used to combine two accumulators into one. It's particularly important in distributed computing scenarios where partial results from different workers/nodes need to be combined into a final result. In parallel processing, different partitions of data might be processed by different workers.

Example:

class CountAccumulator implements Accumulator<Integer> { private int count = 0; public void add(Integer value) { count++; } public void merge(Accumulator other) { // Combine the counts from both accumulators count += ((CountAccumulator)other).count; } }

Seems it's not required in current framework since SQL plugin is running in coordinator node only.

LantaoJin · 2025-03-04T07:59:10Z

core/src/main/java/org/opensearch/sql/calcite/udf/UserDefinedAggFunction.java

+
+  Object result(S accumulator);
+
+  // Add values to the accumulator


use javadoc /** */ before signature.

LantaoJin · 2025-03-04T08:06:26Z

core/src/main/java/org/opensearch/sql/calcite/udf/UserDefinedAggFunction.java

+public interface UserDefinedAggFunction<S extends Accumulator> {
+  S init();
+
+  Object result(S accumulator);


in UDF, the method name is eval, should we change to eval either?

For the function name of agg function, it's defined in calcite here https://github.com/apache/calcite/blob/1793ba79a328c61fb42842f443334cc1353c985f/core/src/main/java/org/apache/calcite/schema/impl/AggregateFunctionImpl.java#L91. We cannot modify them. I will left comment.

LantaoJin · 2025-03-04T08:06:42Z

core/src/main/java/org/opensearch/sql/calcite/udf/UserDefinedFunction.java

+package org.opensearch.sql.calcite.udf;
+
+public interface UserDefinedFunction {
+  Object eval(Object... args);


Add comments.

LantaoJin · 2025-03-04T08:07:26Z

core/src/main/java/org/opensearch/sql/calcite/udf/udaf/TakeAggFunction.java

@@ -0,0 +1,55 @@
+package org.opensearch.sql.calcite.udf.udaf;


copyright header missing

LantaoJin · 2025-03-04T08:09:13Z

core/src/main/java/org/opensearch/sql/calcite/udf/udaf/TakeAggFunction.java

+      return hits;
+    }
+
+    public void add(Object value) {


should add be a part of interface method signature?

I just compare the interface AggregationState. I think we could just make sure we have add in UserDefinedAggFunction. For Accumulator, they can implement their own functions.

LantaoJin · 2025-03-04T08:12:53Z

Add ./gradlew :integ-test:integTest --tests '*Calcite*IT' succeed locally. in your description before the CI issue fixed. @xinyual

LantaoJin · 2025-03-04T08:13:30Z

Related Issues should be addressed

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

LantaoJin · 2025-03-04T08:54:20Z

core/src/main/java/org/opensearch/sql/calcite/udf/UserDefinedAggFunction.java

+   */
+  interface Accumulator {
+    /**
+      * @return the final aggregation value


Have you run ./gradlew spotlessApply before pushing?

Forget to run. Just rerun the push.

LantaoJin · 2025-03-04T08:54:33Z

core/src/main/java/org/opensearch/sql/calcite/udf/UserDefinedAggFunction.java

  S init();

+  /**
+   *


format issue

LantaoJin · 2025-03-04T08:56:18Z

core/src/main/java/org/opensearch/sql/calcite/udf/udaf/TakeAggFunction.java

    @Override
-    public Object result() {
+    public Object value() {
      return hits;
    }


Code indentation problem, please run ./gradlew spotlessApply

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

* add udf/udaf interface and take/sqrt function Signed-off-by: xinyual <xinyual@amazon.com> * add UT Signed-off-by: xinyual <xinyual@amazon.com> * add POW, Atan, Atan2 and corresponding UT Signed-off-by: xinyual <xinyual@amazon.com> * apply spotless Signed-off-by: xinyual <xinyual@amazon.com> * fix table for join it Signed-off-by: xinyual <xinyual@amazon.com> * add java doc Signed-off-by: xinyual <xinyual@amazon.com> * apply spotless Signed-off-by: xinyual <xinyual@amazon.com> --------- Signed-off-by: xinyual <xinyual@amazon.com>

xinyual added 5 commits March 4, 2025 13:58

add udf/udaf interface and take/sqrt function

4cbf2ca

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

add UT

89207a1

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

add POW, Atan, Atan2 and corresponding UT

28869cf

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

apply spotless

d1f8f11

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

fix table for join it

2464c85

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

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 4, 2025 07:47

LantaoJin reviewed Mar 4, 2025

View reviewed changes

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

add java doc

df6728a

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

LantaoJin reviewed Mar 4, 2025

View reviewed changes

apply spotless

958cac9

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

LantaoJin merged commit 6668aa8 into opensearch-project:feature/calcite-engine Mar 4, 2025
5 of 13 checks passed


		Object result(S accumulator);

		// Add values to the accumulator

		@@ -0,0 +1,55 @@
		package org.opensearch.sql.calcite.udf.udaf;

Conversation

xinyual commented Mar 4, 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!

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!

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!

LantaoJin commented Mar 4, 2025

Uh oh!

LantaoJin commented Mar 4, 2025

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xinyual commented Mar 4, 2025 •

edited by LantaoJin

Loading