Remove unused CalciteFuncSignature interface

yuancu · yuancu · commit aff854193f04 · 2025-12-23T15:06:59.000+08:00
Signed-off-by: Yuanchun Shen &lt;yuanchu@amazon.com&gt;
diff --git a/core/src/main/java/org/opensearch/sql/calcite/validate/PplTypeCoercion.java b/core/src/main/java/org/opensearch/sql/calcite/validate/PplTypeCoercion.java
@@ -117,7 +117,7 @@ protected boolean dateTimeStringEquality(
     // - (date, time) -> timestamp
     // - (time, timestamp) -> timestamp
     // - (ip, string) -> ip
-    if (type1 != null & type2 != null) {
+    if (type1 != null && type2 != null) {
       boolean anyNullable = type1.isNullable() || type2.isNullable();
       if ((SqlTypeUtil.isDate(type1) && OpenSearchTypeUtil.isTime(type2))
           || (OpenSearchTypeUtil.isTime(type1) && SqlTypeUtil.isDate(type2))) {
diff --git a/core/src/main/java/org/opensearch/sql/executor/QueryService.java b/core/src/main/java/org/opensearch/sql/executor/QueryService.java
@@ -327,16 +327,16 @@ private RelNode validate(RelNode relNode, CalcitePlanContext context) {
       throw new ExpressionEvaluationException(e.getMessage(), e);
     }
 
-    // 1. Do not remove sort in subqueries so that the orders for queries like `... | sort a |
-    // fields b` is preserved
-    // 2. Disable automatic JSON_TYPE_OPERATOR wrapping for nested JSON functions.
-    // See CALCITE-4989: Calcite wraps nested JSON functions with JSON_TYPE by default
-    // 3. Set hint strategy so that hints can be properly propagated.
-    // See SqlToRelConverter.java#convertSelectImpl
     SqlToRelConverter.Config sql2relConfig =
         SqlToRelConverter.config()
+            // Do not remove sort in subqueries so that the orders for queries like `... | sort a
+            // | fields b` is preserved
             .withRemoveSortInSubQuery(false)
+            // Disable automatic JSON_TYPE_OPERATOR wrapping for nested JSON functions.
+            // See CALCITE-4989: Calcite wraps nested JSON functions with JSON_TYPE by default
             .withAddJsonTypeOperatorEnabled(false)
+            // Set hint strategy so that hints can be properly propagated.
+            // See SqlToRelConverter.java#convertSelectImpl
             .withHintStrategyTable(PPLHintStrategyTable.getHintStrategyTable());
     SqlToRelConverter sql2rel =
         new PplSqlToRelConverter(
diff --git a/core/src/main/java/org/opensearch/sql/expression/function/CalciteFuncSignature.java b/core/src/main/java/org/opensearch/sql/expression/function/CalciteFuncSignature.java
diff --git a/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java b/core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java
@@ -278,7 +278,6 @@
 import org.apache.calcite.sql.validate.SqlUserDefinedAggFunction;
 import org.apache.calcite.sql.validate.SqlUserDefinedFunction;
 import org.apache.calcite.tools.RelBuilder;
-import org.apache.commons.lang3.tuple.Pair;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.opensearch.sql.calcite.CalcitePlanContext;
@@ -357,42 +356,38 @@ default RexNode resolve(RexBuilder builder, RexNode... args) {
    * implementations are independent of any specific data storage, should be registered here
    * internally.
    */
-  private final ImmutableMap<BuiltinFunctionName, Pair<CalciteFuncSignature, FunctionImp>>
-      functionRegistry;
+  private final ImmutableMap<BuiltinFunctionName, FunctionImp> functionRegistry;
 
   /**
    * The external function registry. Functions whose implementations depend on a specific data
    * engine should be registered here. This reduces coupling between the core module and particular
    * storage backends.
    */
-  private final Map<BuiltinFunctionName, Pair<CalciteFuncSignature, FunctionImp>>
-      externalFunctionRegistry;
+  private final Map<BuiltinFunctionName, FunctionImp> externalFunctionRegistry;
 
   /**
    * The registry for built-in agg functions. Agg Functions defined by the PPL specification, whose
    * implementations are independent of any specific data storage, should be registered here
    * internally.
    */
-  private final ImmutableMap<BuiltinFunctionName, Pair<CalciteFuncSignature, AggHandler>>
-      aggFunctionRegistry;
+  private final ImmutableMap<BuiltinFunctionName, AggHandler> aggFunctionRegistry;
 
   /**
    * The external agg function registry. Agg Functions whose implementations depend on a specific
    * data engine should be registered here. This reduces coupling between the core module and
    * particular storage backends.
    */
-  private final Map<BuiltinFunctionName, Pair<CalciteFuncSignature, AggHandler>>
-      aggExternalFunctionRegistry;
+  private final Map<BuiltinFunctionName, AggHandler> aggExternalFunctionRegistry;
 
   private PPLFuncImpTable(Builder builder, AggBuilder aggBuilder) {
-    final ImmutableMap.Builder<BuiltinFunctionName, Pair<CalciteFuncSignature, FunctionImp>>
-        mapBuilder = ImmutableMap.builder();
+    final ImmutableMap.Builder<BuiltinFunctionName, FunctionImp> mapBuilder =
+        ImmutableMap.builder();
     mapBuilder.putAll(builder.map);
     this.functionRegistry = ImmutableMap.copyOf(mapBuilder.build());
     this.externalFunctionRegistry = new ConcurrentHashMap<>();
 
-    final ImmutableMap.Builder<BuiltinFunctionName, Pair<CalciteFuncSignature, AggHandler>>
-        aggMapBuilder = ImmutableMap.builder();
+    final ImmutableMap.Builder<BuiltinFunctionName, AggHandler> aggMapBuilder =
+        ImmutableMap.builder();
     aggMapBuilder.putAll(aggBuilder.map);
     this.aggFunctionRegistry = ImmutableMap.copyOf(aggMapBuilder.build());
     this.aggExternalFunctionRegistry = new ConcurrentHashMap<>();
@@ -405,14 +400,11 @@ private PPLFuncImpTable(Builder builder, AggBuilder aggBuilder) {
    * @param operator a SqlOperator representing an externally implemented function
    */
   public void registerExternalOperator(BuiltinFunctionName functionName, SqlOperator operator) {
-    CalciteFuncSignature signature =
-        new CalciteFuncSignature(functionName.getName(), operator.getOperandTypeChecker());
     if (externalFunctionRegistry.containsKey(functionName)) {
       logger.warn(
           String.format(Locale.ROOT, "Function %s is registered multiple times", functionName));
     }
-    externalFunctionRegistry.put(
-        functionName, Pair.of(signature, (builder, args) -> builder.makeCall(operator, args)));
+    externalFunctionRegistry.put(functionName, (builder, args) -> builder.makeCall(operator, args));
   }
 
   /**
@@ -429,13 +421,11 @@ public void registerExternalAggOperator(
           String.format(
               Locale.ROOT, "Aggregate function %s is registered multiple times", functionName));
     }
-    CalciteFuncSignature signature =
-        new CalciteFuncSignature(functionName.getName(), aggFunction.getOperandTypeChecker());
     AggHandler handler =
         (distinct, field, argList, ctx) ->
             UserDefinedFunctionUtils.makeAggregateCall(
                 aggFunction, List.of(field), argList, ctx.relBuilder);
-    aggExternalFunctionRegistry.put(functionName, Pair.of(signature, handler));
+    aggExternalFunctionRegistry.put(functionName, handler);
   }
 
   public RelBuilder.AggCall resolveAgg(
@@ -445,12 +435,10 @@ public RelBuilder.AggCall resolveAgg(
       List<RexNode> argList,
       CalcitePlanContext context) {
     var implementation = getImplementation(functionName);
-    var handler = implementation.getValue();
-    return handler.apply(distinct, field, argList, context);
+    return implementation.apply(distinct, field, argList, context);
   }
 
-  private Pair<CalciteFuncSignature, AggHandler> getImplementation(
-      BuiltinFunctionName functionName) {
+  private AggHandler getImplementation(BuiltinFunctionName functionName) {
     var implementation = aggExternalFunctionRegistry.get(functionName);
     if (implementation == null) {
       implementation = aggFunctionRegistry.get(functionName);
@@ -474,7 +462,7 @@ public RexNode resolve(
     // Check the external function registry first. This allows the data-storage-dependent
     // function implementations to override the internal ones with the same name.
     //  If the function is not part of the external registry, check the internal registry.
-    Pair<CalciteFuncSignature, FunctionImp> implementation =
+    FunctionImp implementation =
         externalFunctionRegistry.get(functionName) != null
             ? externalFunctionRegistry.get(functionName)
             : functionRegistry.get(functionName);
@@ -486,7 +474,7 @@ public RexNode resolve(
     // For example, the REDUCE function requires the second argument to be cast to the
     // return type of the lambda function.
     compulsoryCast(builder, functionName, args);
-    return implementation.getValue().resolve(builder, args);
+    return implementation.resolve(builder, args);
   }
 
   /**
@@ -1092,39 +1080,33 @@ void populate() {
   }
 
   private static class Builder extends AbstractBuilder {
-    private final Map<BuiltinFunctionName, Pair<CalciteFuncSignature, FunctionImp>> map =
-        new HashMap<>();
+    private final Map<BuiltinFunctionName, FunctionImp> map = new HashMap<>();
 
     @Override
     void register(
         BuiltinFunctionName functionName,
         FunctionImp implement,
         SqlOperandTypeChecker typeChecker) {
-      CalciteFuncSignature signature =
-          new CalciteFuncSignature(functionName.getName(), typeChecker);
       if (map.containsKey(functionName)) {
         throw new IllegalStateException(
             String.format(
                 Locale.ROOT,
                 "Each function can only be registered with one operator: %s",
                 functionName));
       }
-      map.put(functionName, Pair.of(signature, implement));
+      map.put(functionName, implement);
     }
   }
 
   private static class AggBuilder {
     private static final double MEDIAN_PERCENTILE = 50.0;
-    private final Map<BuiltinFunctionName, Pair<CalciteFuncSignature, AggHandler>> map =
-        new HashMap<>();
+    private final Map<BuiltinFunctionName, AggHandler> map = new HashMap<>();
 
     void register(
         BuiltinFunctionName functionName,
         AggHandler aggHandler,
         SqlOperandTypeChecker typeChecker) {
-      CalciteFuncSignature signature =
-          new CalciteFuncSignature(functionName.getName(), typeChecker);
-      map.put(functionName, Pair.of(signature, aggHandler));
+      map.put(functionName, aggHandler);
     }
 
     void registerOperator(BuiltinFunctionName functionName, SqlAggFunction aggFunction) {
diff --git a/core/src/test/java/org/opensearch/sql/expression/function/AggFunctionTestBase.java b/core/src/test/java/org/opensearch/sql/expression/function/AggFunctionTestBase.java
@@ -16,22 +16,19 @@
 public abstract class AggFunctionTestBase {
 
   @SuppressWarnings("unchecked")
-  protected Map<BuiltinFunctionName, org.apache.commons.lang3.tuple.Pair<?, AggHandler>>
-      getAggFunctionRegistry() {
+  protected Map<BuiltinFunctionName, AggHandler> getAggFunctionRegistry() {
     try {
       PPLFuncImpTable funcTable = PPLFuncImpTable.INSTANCE;
       Field field = PPLFuncImpTable.class.getDeclaredField("aggFunctionRegistry");
       field.setAccessible(true);
-      return (Map<BuiltinFunctionName, org.apache.commons.lang3.tuple.Pair<?, AggHandler>>)
-          field.get(funcTable);
+      return (Map<BuiltinFunctionName, AggHandler>) field.get(funcTable);
     } catch (Exception e) {
       throw new RuntimeException("Failed to access aggFunctionRegistry", e);
     }
   }
 
   protected void assertFunctionIsRegistered(BuiltinFunctionName functionName) {
-    Map<BuiltinFunctionName, org.apache.commons.lang3.tuple.Pair<?, AggHandler>> registry =
-        getAggFunctionRegistry();
+    Map<BuiltinFunctionName, AggHandler> registry = getAggFunctionRegistry();
     assertTrue(
         registry.containsKey(functionName),
         functionName.getName().getFunctionName()
@@ -48,36 +45,23 @@ protected void assertFunctionsAreRegistered(BuiltinFunctionName... functionNames
   }
 
   protected void assertFunctionHandlerTypes(BuiltinFunctionName... functionNames) {
-    Map<BuiltinFunctionName, org.apache.commons.lang3.tuple.Pair<?, AggHandler>> registry =
-        getAggFunctionRegistry();
+    Map<BuiltinFunctionName, AggHandler> registry = getAggFunctionRegistry();
     for (BuiltinFunctionName functionName : functionNames) {
-      org.apache.commons.lang3.tuple.Pair<?, AggHandler> registryEntry = registry.get(functionName);
-      assertNotNull(
-          registryEntry, functionName.getName().getFunctionName() + " should be registered");
-
-      // Extract the AggHandler from the pair
-      AggHandler handler = registryEntry.getRight();
-
+      AggHandler handler = registry.get(functionName);
       assertNotNull(
           handler, functionName.getName().getFunctionName() + " handler should not be null");
-      assertTrue(
-          handler instanceof AggHandler,
-          functionName.getName().getFunctionName()
-              + " handler should implement AggHandler interface");
     }
   }
 
   protected void assertRegistryMinimumSize(int expectedMinimumSize) {
-    Map<BuiltinFunctionName, org.apache.commons.lang3.tuple.Pair<?, AggHandler>> registry =
-        getAggFunctionRegistry();
+    Map<BuiltinFunctionName, AggHandler> registry = getAggFunctionRegistry();
     assertTrue(
         registry.size() >= expectedMinimumSize,
         "Registry should contain at least " + expectedMinimumSize + " aggregate functions");
   }
 
   protected void assertKnownFunctionsPresent(Set<BuiltinFunctionName> knownFunctions) {
-    Map<BuiltinFunctionName, org.apache.commons.lang3.tuple.Pair<?, AggHandler>> registry =
-        getAggFunctionRegistry();
+    Map<BuiltinFunctionName, AggHandler> registry = getAggFunctionRegistry();
     long foundFunctions = registry.keySet().stream().filter(knownFunctions::contains).count();
 
     assertTrue(
