feat(isthmus): support for SQL TRIM function (#401)

Author: bestbeforetoday

build.gradle.kts

@@ -1,6 +1,7 @@
 import com.diffplug.gradle.spotless.SpotlessExtension
 import com.diffplug.gradle.spotless.SpotlessPlugin
 import com.github.vlsi.gradle.dsl.configureEach
+import org.gradle.api.tasks.testing.logging.TestExceptionFormat
 
 plugins {
   `maven-publish`
@@ -43,6 +44,7 @@ allprojects {
     val javaToolchains = project.extensions.getByType<JavaToolchainService>()
     useJUnitPlatform()
     javaLauncher.set(javaToolchains.launcherFor { languageVersion.set(JavaLanguageVersion.of(11)) })
+    testLogging { exceptionFormat = TestExceptionFormat.FULL }
   }
   tasks.withType<JavaCompile> {
     sourceCompatibility = "17"

isthmus/src/main/java/io/substrait/isthmus/expression/EnumConverter.java

@@ -14,6 +14,7 @@
 import org.apache.calcite.rex.RexBuilder;
 import org.apache.calcite.rex.RexLiteral;
 import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.fun.SqlTrimFunction;
 import org.apache.calcite.sql.type.SqlTypeName;
 
 /**
@@ -52,15 +53,38 @@ public class EnumConverter {
     calciteEnumMap.put(
         argAnchor(DefaultExtensionCatalog.FUNCTIONS_DATETIME, "extract:req_time", 0),
         TimeUnitRange.class);
+
+    calciteEnumMap.put(
+        argAnchor(DefaultExtensionCatalog.FUNCTIONS_STRING, "trim:vchar_vchar", 0),
+        SqlTrimFunction.Flag.class);
+    calciteEnumMap.put(
+        argAnchor(DefaultExtensionCatalog.FUNCTIONS_STRING, "trim:str_str", 0),
+        SqlTrimFunction.Flag.class);
+    calciteEnumMap.put(
+        argAnchor(DefaultExtensionCatalog.FUNCTIONS_STRING, "ltrim:vchar_vchar", 0),
+        SqlTrimFunction.Flag.class);
+    calciteEnumMap.put(
+        argAnchor(DefaultExtensionCatalog.FUNCTIONS_STRING, "ltrim:str_str", 0),
+        SqlTrimFunction.Flag.class);
+    calciteEnumMap.put(
+        argAnchor(DefaultExtensionCatalog.FUNCTIONS_STRING, "rtrim:vchar_vchar", 0),
+        SqlTrimFunction.Flag.class);
+    calciteEnumMap.put(
+        argAnchor(DefaultExtensionCatalog.FUNCTIONS_STRING, "rtrim:str_str", 0),
+        SqlTrimFunction.Flag.class);
   }
 
   private static Optional<Enum<?>> constructValue(
       Class<? extends Enum<?>> cls, Supplier<Optional<String>> option) {
     if (cls.isAssignableFrom(TimeUnitRange.class)) {
       return option.get().map(TimeUnitRange::valueOf);
-    } else {
-      return Optional.empty();
     }
+
+    if (cls.isAssignableFrom(SqlTrimFunction.Flag.class)) {
+      return option.get().map(SqlTrimFunction.Flag::valueOf);
+    }
+
+    return Optional.empty();
   }
 
   static Optional<RexLiteral> toRex(
@@ -123,9 +147,7 @@ static boolean canConvert(Enum<?> value) {
   }
 
   static boolean isEnumValue(RexNode value) {
-    return value != null
-        && (value instanceof RexLiteral)
-        && value.getType().getSqlTypeName() == SqlTypeName.SYMBOL;
+    return value instanceof RexLiteral && value.getType().getSqlTypeName() == SqlTypeName.SYMBOL;
   }
 
   private static class ArgAnchor {

isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java

@@ -363,11 +363,11 @@ public RexNode visit(Expression.ScalarFunctionInvocation expr) throws RuntimeExc
                         callConversionFailureMessage(
                             "scalar", expr.declaration().name(), expr.arguments())));
 
-    var eArgs = expr.arguments();
+    var eArgs = scalarFunctionConverter.getExpressionArguments(expr);
     var args =
-        IntStream.range(0, expr.arguments().size())
+        IntStream.range(0, eArgs.size())
             .mapToObj(i -> eArgs.get(i).accept(expr.declaration(), i, this))
-            .collect(java.util.stream.Collectors.toList());
+            .collect(Collectors.toList());
 
     RelDataType returnType = typeConverter.toCalcite(typeFactory, expr.outputType());
     return rexBuilder.makeCall(returnType, operator, args);
@@ -395,9 +395,9 @@ public RexNode visit(Expression.WindowFunctionInvocation expr) throws RuntimeExc
 
     List<FunctionArg> eArgs = expr.arguments();
     List<RexNode> args =
-        IntStream.range(0, expr.arguments().size())
+        IntStream.range(0, eArgs.size())
             .mapToObj(i -> eArgs.get(i).accept(expr.declaration(), i, this))
-            .collect(java.util.stream.Collectors.toList());
+            .collect(Collectors.toList());
 
     List<RexNode> partitionKeys =
         expr.partitionBy().stream().map(e -> e.accept(this)).collect(Collectors.toList());

isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java

@@ -67,7 +67,7 @@ public FunctionConverter(
     signatures.addAll(additionalSignatures);
     signatures.addAll(getSigs());
     this.typeFactory = typeFactory;
-    this.substraitFuncKeyToSqlOperatorMap = ArrayListMultimap.<String, SqlOperator>create();
+    this.substraitFuncKeyToSqlOperatorMap = ArrayListMultimap.create();
 
     var alm = ArrayListMultimap.<String, F>create();
     for (var f : functions) {
@@ -78,22 +78,19 @@ public FunctionConverter(
         signatures.stream()
             .collect(
                 Multimaps.toMultimap(
-                    FunctionMappings.Sig::name, f -> f, () -> ArrayListMultimap.create()));
+                    FunctionMappings.Sig::name, Function.identity(), ArrayListMultimap::create));
     var matcherMap = new IdentityHashMap<SqlOperator, FunctionFinder>();
     for (String key : alm.keySet()) {
       var sigs = calciteOperators.get(key);
-      if (sigs == null) {
-        logger.atInfo().log("Dropping function due to no binding: {}", key);
-        continue;
+      if (sigs.isEmpty()) {
+        logger.atInfo().log("No binding for function: {}", key);
       }
 
       for (var sig : sigs) {
         var implList = alm.get(key);
-        if (implList == null || implList.isEmpty()) {
-          continue;
+        if (!implList.isEmpty()) {
+          matcherMap.put(sig.operator(), new FunctionFinder(key, sig.operator(), implList));
         }
-
-        matcherMap.put(sig.operator(), new FunctionFinder(key, sig.operator(), implList));
       }
     }
 
@@ -110,14 +107,16 @@ public FunctionConverter(
 
   public Optional<SqlOperator> getSqlOperatorFromSubstraitFunc(String key, Type outputType) {
     var resolver = getTypeBasedResolver();
-    if (!substraitFuncKeyToSqlOperatorMap.containsKey(key)) {
+    var operators = substraitFuncKeyToSqlOperatorMap.get(key);
+    if (operators.isEmpty()) {
       return Optional.empty();
     }
-    var operators = substraitFuncKeyToSqlOperatorMap.get(key);
+
     // only one SqlOperator is possible
     if (operators.size() == 1) {
       return Optional.of(operators.iterator().next());
     }
+
     // at least 2 operators. Use output type to resolve SqlOperator.
     String outputTypeStr = outputType.accept(ToTypeString.INSTANCE);
     var resolvedOperators =
@@ -146,15 +145,15 @@ private Map<SqlOperator, FunctionMappings.TypeBasedResolver> getTypeBasedResolve
   protected abstract ImmutableList<FunctionMappings.Sig> getSigs();
 
   protected class FunctionFinder {
-    private final String name;
+    private final String substraitName;
     private final SqlOperator operator;
     private final List<F> functions;
     private final Map<String, F> directMap;
     private final Optional<SingularArgumentMatcher<F>> singularInputType;
     private final Util.IntRange argRange;
 
-    public FunctionFinder(String name, SqlOperator operator, List<F> functions) {
-      this.name = name;
+    public FunctionFinder(String substraitName, SqlOperator operator, List<F> functions) {
+      this.substraitName = substraitName;
       this.operator = operator;
       this.functions = functions;
       this.argRange =
@@ -167,7 +166,7 @@ public FunctionFinder(String name, SqlOperator operator, List<F> functions) {
         String key = func.key();
         directMap.put(key, func);
         if (func.requiredArguments().size() != func.args().size()) {
-          directMap.put(F.constructKey(name, func.requiredArguments()), func);
+          directMap.put(F.constructKey(substraitName, func.requiredArguments()), func);
         }
       }
       this.directMap = directMap.build();
@@ -357,7 +356,7 @@ public Optional<T> attemptMatch(C call, Function<RexNode, Expression> topLevelCo
 
       Optional<String> directMatchKey =
           possibleKeys
-              .map(argList -> name + ":" + argList)
+              .map(argList -> substraitName + ":" + argList)
               .filter(directMap::containsKey)
               .findFirst();
 
@@ -438,8 +437,8 @@ private Optional<T> matchCoerced(C call, Type outputType, List<Expression> expre
       return Optional.of(generateBinding(call, matchFunction.get(), coercedArgs, outputType));
     }
 
-    protected String getName() {
-      return name;
+    protected String getSubstraitName() {
+      return substraitName;
     }
 
     public SqlOperator getOperator() {