[Calcite Engine] Function framework refactoring by qianheng-aws · Pull Request #3522 · opensearch-project/sql

qianheng-aws · 2025-04-07T10:43:55Z

Description

Do refactoring for the framework of function.

It includes change or enhancement:

Add PPLFuncImpTable for registering logical-level implementation for resolving a BuiltinFunctionName. It also includes resolving for functions with different parameters by using CalciteFuncSignature.
Add PPLBuiltinOperators for all self-implemented UDFs. This class is a physical-level implementation of an operator.
Migrate all SqlStdOperatorTable and SqlLibraryOperators to the new framework, and also migrate the span function and typeof function.
Make the UDF static instead of creating a new one when calling visitFunction.
Replace function name(STRING) with BuiltinFunctionName(ENUM) to strengthen code robustness

The new framework aims to hide all implementation at the physical level, so the plan will be more concise and clear.

Take span for example: for a time-related field, span will use a ScalarFunctionImpl (physical-level implementation, can only see span(\$1, \$2, \$3) in the plan); while using logical implementation like FLOOR($1 / $2) * $2 for a non-time field. This makes the plan weird and confusing, and it also blocks push-down since the push-down rule takes effect by identifying the span function operator. After migrating, the SPAN operator is able to handle both cases internally.

PPLFuncImpTable will provide more flexible implementation of a function than only using SqlOperator.

Take typeof for example, after migrating, the new implementation will replace using ScalarFunction TYPEOF(\$1) with returning a RexLiteral directly, which avoids row-by-row evaluation.

Related Issues

Resolves #3493

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: Heng Qian <qianheng@amazon.com>

penghuo · 2025-04-08T00:32:04Z

+    for (Map.Entry<CalciteFuncSignature, FunctionImp> implement : implementList) {
+      if (implement.getKey().match(functionName.getName(), argTypes)) {
+        return implement.getValue();
+      }
+    }


In worst case, what is the maximum number of signatures that would need to be checked?

The maximum is 2 only for LOG, while others always have only 1. LOG has 2 implementation -- LOG($1) and LOG($1, $2), the former is translated to LOG($1, E).

And we expect different implementation of one function should be included in the implementor of its related operator as much as possible, i.e. implementation details should be hidden in the physical-level instead of logical-level, unless we want to expose the logical detail of the implementation on purpose.(like if want integral division for integer, we may need the new added cast calling is exposed in the plan instead of being hidden in the implementor).

So in the visible future, the maximum number should be only 1 or very small. And we can deprecate this extra level if we strictly follow the above rule and requires only 1-to-1 mapping between function to operator.

penghuo · 2025-04-08T00:32:37Z

+import org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.calcite.sql.validate.SqlUserDefinedFunction;
+
+public interface UserDefinedFunctionHelper {


Why a helper an interface? Can be UserDefinedFunctionFactory or `UserDefinedFunctionTemplate?

penghuo · 2025-04-08T00:32:46Z

+  default SqlOperandMetadata getOperandMetadata() {
+    return null;
+  }
+  ;


penghuo · 2025-04-08T00:45:12Z

+              case EXPR_TIME -> "evalTime";
+              case EXPR_TIMESTAMP -> "evalTimestamp";
+              default -> throw new IllegalArgumentException(
+                  String.format("Unsupported expr udt: %s", exprSqlType.getUdt()));


UDT is internal implementation, we should hide internal error message from user.

penghuo · 2025-04-08T00:45:34Z

+                        SpanFunctionImpl.class, methodName, String.class, int.class, String.class));
+        return function.getImplementor().implement(translator, call, RexImpTable.NullAs.NULL);
+      }
+      throw new IllegalArgumentException(String.format("Unsupported RelDataType: %s", fieldType));


RelDataType is internal implementation, we should hide internal error message from user.

xinyual · 2025-04-08T03:29:51Z

+import org.apache.calcite.rel.type.RelDataType;
+
+/** Function signature is composed by function name and arguments list. */
+public record CalciteFuncSignature(FunctionName functionName, List<RelDataType> funcArgTypes) {


Do we really need need this signature? If we follow the SpanFunctionImpl, we could put all real implement function like evalDate into same class. Also, the argument validation check would be in another PR. I think the only reason here is about implicit conversion. But I think it's already disabled in calcite now?

I'm wondering if there is chance that:

an function may has different operator depends on params' type or size and wants this information exposed on our plan. Although, by most cases, we expect the one-to-one relationship between function and operator and hide different processing logic in one operator.

different functions map to one operator while each of them has their specific limitation on the params' size or type. E.g. We use operator NOT_EQUAL for both function != and XOR, while XOR has its limitation that only accepting boolean, so we could put this limitation check on its signature. Of course, we can implement XOR by an UDF with more efforts. It depends on whether we want to expose this implementation details to customer in the plan or not.

It stills remains discussion on the above 2 cases. We can keep this redundant level here since it doesn't harm the functionality or performance, until the final decision in the end.

xinyual · 2025-04-08T03:41:43Z

+import org.apache.calcite.schema.impl.ScalarFunctionImpl;
+
+/** The UDF which implements its functionality by invoking the `eval` method of a class */
+public abstract class ScalarUDF implements UserDefinedFunctionHelper {


Almost all UDF functions has different implementations for different arguments. So I think this class is useless?

Yeah, useless for now. I'm not sure if it could be useful in the future.

This is for the case where one reflective method could cover the functionality of a operator, like the BuiltInMethod in calcite. For more complex implementation like handling different arguments needs different logic, should use ImplementorUDF

LantaoJin · 2025-04-08T06:20:48Z

+   * all private Use method directly in {@link BuiltInMethod} if possible, most operators'
+   * implementor could be substituted by a single method.
+   */
+  private static Expression invokeCalciteImplementor(


How does this method invoke with private static

LantaoJin · 2025-04-08T06:45:48Z

+    }
+  }
+
+  @Strict


This annotation applied to a user-defined function that indicates that the function returns null if and only if one or more of its arguments are null. please confirm

Yes, I think that satisfy our expectation.

Signed-off-by: Heng Qian <qianheng@amazon.com>

…function-refactoring # Conflicts: # core/src/main/java/org/opensearch/sql/calcite/utils/BuiltinFunctionUtils.java # core/src/main/java/org/opensearch/sql/calcite/utils/UserDefinedFunctionUtils.java

Signed-off-by: Heng Qian <qianheng@amazon.com>

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

[Calcite Engine] Function framework refactoring

b167cdf

Signed-off-by: Heng Qian <qianheng@amazon.com>

qianheng-aws 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 April 7, 2025 10:43

[Calcite Engine] migrate TYPEOF

f2ea91e

Signed-off-by: Heng Qian <qianheng@amazon.com>

noCharger added the calcite calcite migration releated label Apr 7, 2025

penghuo reviewed Apr 8, 2025

View reviewed changes

xinyual reviewed Apr 8, 2025

View reviewed changes

yuancu mentioned this pull request Apr 8, 2025

Validate arguments for datetime, string, and math UDFs #3512

Closed

8 tasks

xinyual reviewed Apr 8, 2025

View reviewed changes

LantaoJin reviewed Apr 8, 2025

View reviewed changes

qianheng-aws added 3 commits April 8, 2025 17:58

[Calcite Engine] Address comments and fix UT

86a1d82

Signed-off-by: Heng Qian <qianheng@amazon.com>

[Calcite Engine] Address comments

9cfa98a

Signed-off-by: Heng Qian <qianheng@amazon.com>

Merge remote-tracking branch 'refs/remotes/origin/main' into calcite-…

370c0a3

…function-refactoring # Conflicts: # core/src/main/java/org/opensearch/sql/calcite/utils/BuiltinFunctionUtils.java # core/src/main/java/org/opensearch/sql/calcite/utils/UserDefinedFunctionUtils.java

[Calcite Engine] Fix after merging main

1a00b0b

Signed-off-by: Heng Qian <qianheng@amazon.com>

qianheng-aws requested review from LantaoJin, penghuo and xinyual April 9, 2025 03:56

LantaoJin approved these changes Apr 9, 2025

View reviewed changes

penghuo approved these changes Apr 9, 2025

View reviewed changes

penghuo merged commit b0384a8 into opensearch-project:main Apr 9, 2025
22 checks passed

LantaoJin mentioned this pull request Apr 25, 2025

Migrate existing UDFs to PPLFuncImpTable #3576

Merged

8 tasks

penghuo pushed a commit that referenced this pull request Jun 16, 2025

[Calcite Engine] Function framework refactoring (#3522)

dbe6871

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

qianheng-aws deleted the calcite-function-refactoring branch March 26, 2026 10:05

+                  }
+                }
+                @Strict

Conversation

qianheng-aws commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qianheng-aws Apr 8, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

5 participants

qianheng-aws commented Apr 7, 2025 •

edited

Loading

qianheng-aws Apr 8, 2025 •

edited

Loading