fix(isthmus): concat of different string types

The type matching used when mapping Calcite to Substrait expressions
required parameters and return type of the concat function to be either
all varchar or all string. Any mixing of types caused a failure.

Signed-off-by: Mark S. Lewis <[email protected]>

Author: bestbeforetoday

isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java

@@ -39,6 +39,11 @@ public int getMaxNumericPrecision() {
     return 38;
   }
 
+  @Override
+  public boolean shouldConvertRaggedUnionTypesToVarying() {
+    return true;
+  }
+
   public static RelDataTypeFactory createTypeFactory() {
     return new JavaTypeFactoryImpl(TYPE_SYSTEM);
   }

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

@@ -52,7 +52,7 @@ public AggregateFunctionConverter(
   protected AggregateFunctionInvocation generateBinding(
       WrappedAggregateCall call,
       SimpleExtension.AggregateFunctionVariant function,
-      List<FunctionArg> arguments,
+      List<? extends FunctionArg> arguments,
       Type outputType) {
     AggregateCall agg = call.getUnderlying();
 

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

@@ -25,6 +25,7 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
@@ -148,7 +149,6 @@ protected class FunctionFinder {
     private final SqlOperator operator;
     private final List<F> functions;
     private final Map<String, F> directMap;
-    private final SignatureMatcher<F> matcher;
     private final Optional<SingularArgumentMatcher<F>> singularInputType;
     private final Util.IntRange argRange;
 
@@ -160,7 +160,6 @@ public FunctionFinder(String name, SqlOperator operator, List<F> functions) {
           Util.IntRange.of(
               functions.stream().mapToInt(t -> t.getRange().getStartInclusive()).min().getAsInt(),
               functions.stream().mapToInt(t -> t.getRange().getEndExclusive()).max().getAsInt());
-      this.matcher = getSignatureMatcher(operator, functions);
       this.singularInputType = getSingularInputType(functions);
       var directMap = ImmutableMap.<String, F>builder();
       for (var func : functions) {
@@ -177,21 +176,18 @@ public boolean allowedArgCount(int count) {
       return argRange.within(count);
     }
 
-    private static <F extends SimpleExtension.Function> SignatureMatcher<F> getSignatureMatcher(
-        SqlOperator operator, List<F> functions) {
-      return (inputTypes, outputType) -> {
-        for (F function : functions) {
-          List<SimpleExtension.Argument> args = function.requiredArguments();
-          // Make sure that arguments & return are within bounds and match the types
-          if (function.returnType() instanceof ParameterizedType
-              && isMatch(outputType, (ParameterizedType) function.returnType())
-              && inputTypesSatisfyDefinedArguments(inputTypes, args)) {
-            return Optional.of(function);
-          }
+    private Optional<F> signatureMatch(List<Type> inputTypes, Type outputType) {
+      for (F function : functions) {
+        List<SimpleExtension.Argument> args = function.requiredArguments();
+        // Make sure that arguments & return are within bounds and match the types
+        if (function.returnType() instanceof ParameterizedType
+            && isMatch(outputType, (ParameterizedType) function.returnType())
+            && inputTypesMatchDefinedArguments(inputTypes, args)) {
+          return Optional.of(function);
         }
+      }
 
-        return Optional.empty();
-      };
+      return Optional.empty();
     }
 
     /**
@@ -207,7 +203,7 @@ && inputTypesSatisfyDefinedArguments(inputTypes, args)) {
      * @param args expected arguments as defined in a {@link SimpleExtension.Function}
      * @return true if the {@code inputTypes} satisfy the {@code args}, false otherwise
      */
-    private static boolean inputTypesSatisfyDefinedArguments(
+    private static boolean inputTypesMatchDefinedArguments(
         List<Type> inputTypes, List<SimpleExtension.Argument> args) {
 
       Map<String, Set<Type>> wildcardToType = new HashMap<>();
@@ -317,7 +313,7 @@ private Stream<String> matchKeys(List<RexNode> rexOperands, List<String> opTypes
 
       assert (rexOperands.size() == opTypes.size());
 
-      if (rexOperands.size() == 0) {
+      if (rexOperands.isEmpty()) {
         return Stream.of("");
       } else {
         List<List<String>> argTypeLists =
@@ -346,23 +342,20 @@ public Optional<T> attemptMatch(C call, Function<RexNode, Expression> topLevelCo
        * Once a FunctionVariant is resolved we can map the String Literal
        * to a EnumArg.
        */
-      var operands =
-          call.getOperands().map(topLevelConverter).collect(java.util.stream.Collectors.toList());
-      var opTypes =
-          operands.stream().map(Expression::getType).collect(java.util.stream.Collectors.toList());
+      var operands = call.getOperands().map(topLevelConverter).collect(Collectors.toList());
+      var opTypes = operands.stream().map(Expression::getType).collect(Collectors.toList());
 
       var outputType = typeConverter.toSubstrait(call.getType());
 
       // try to do a direct match
       var typeStrings =
           opTypes.stream().map(t -> t.accept(ToTypeString.INSTANCE)).collect(Collectors.toList());
-      var possibleKeys =
-          matchKeys(call.getOperands().collect(java.util.stream.Collectors.toList()), typeStrings);
+      var possibleKeys = matchKeys(call.getOperands().collect(Collectors.toList()), typeStrings);
 
       var directMatchKey =
           possibleKeys
               .map(argList -> name + ":" + argList)
-              .filter(k -> directMap.containsKey(k))
+              .filter(directMap::containsKey)
               .findFirst();
 
       if (directMatchKey.isPresent()) {
@@ -375,13 +368,13 @@ public Optional<T> attemptMatch(C call, Function<RexNode, Expression> topLevelCo
                     operands.stream(),
                     (r, o) -> {
                       if (EnumConverter.isEnumValue(r)) {
-                        return EnumConverter.fromRex(variant, (RexLiteral) r).orElseGet(() -> null);
+                        return EnumConverter.fromRex(variant, (RexLiteral) r).orElse(null);
                       } else {
                         return o;
                       }
                     })
-                .collect(java.util.stream.Collectors.toList());
-        var allArgsMapped = funcArgs.stream().filter(e -> e == null).findFirst().isEmpty();
+                .collect(Collectors.toList());
+        var allArgsMapped = funcArgs.stream().filter(Objects::isNull).findFirst().isEmpty();
         if (allArgsMapped) {
           return Optional.of(generateBinding(call, variant, funcArgs, outputType));
         } else {
@@ -411,53 +404,35 @@ private Optional<T> matchByLeastRestrictive(
         return Optional.empty();
       }
       Type type = typeConverter.toSubstrait(leastRestrictive);
-      var out = singularInputType.get().tryMatch(type, outputType);
-
-      if (out.isPresent()) {
-        var declaration = out.get();
-        var coercedArgs = coerceArguments(operands, type);
-        declaration.validateOutputType(coercedArgs, outputType);
-        return Optional.of(
-            generateBinding(
-                call,
-                out.get(),
-                coercedArgs.stream().map(FunctionArg.class::cast).collect(Collectors.toList()),
-                outputType));
-      }
-      return Optional.empty();
+      var out = singularInputType.orElseThrow().tryMatch(type, outputType);
+
+      return out.map(
+          declaration -> {
+            var coercedArgs = coerceArguments(operands, type);
+            declaration.validateOutputType(coercedArgs, outputType);
+            return generateBinding(call, out.get(), coercedArgs, outputType);
+          });
     }
 
-    private Optional<T> matchCoerced(C call, Type outputType, List<Expression> operands) {
-
+    private Optional<T> matchCoerced(C call, Type outputType, List<Expression> expressions) {
       // Convert the operands to the proper Substrait type
-      List<Type> allTypes =
+      List<Type> operandTypes =
           call.getOperands()
               .map(RexNode::getType)
               .map(typeConverter::toSubstrait)
               .collect(Collectors.toList());
 
-      // See if all the input types match the function
-      Optional<F> matchFunction = this.matcher.tryMatch(allTypes, outputType);
-      if (matchFunction.isPresent()) {
-        List<Expression> coerced =
-            Streams.zip(
-                    operands.stream(),
-                    call.getOperands(),
-                    (a, b) -> {
-                      Type type = typeConverter.toSubstrait(b.getType());
-                      return coerceArgument(a, type);
-                    })
-                .collect(Collectors.toList());
-
-        return Optional.of(
-            generateBinding(
-                call,
-                matchFunction.get(),
-                coerced.stream().map(FunctionArg.class::cast).collect(Collectors.toList()),
-                outputType));
+      // See if all the input types can be made to match the function
+      Optional<F> matchFunction = signatureMatch(operandTypes, outputType);
+      if (matchFunction.isEmpty()) {
+        return Optional.empty();
       }
 
-      return Optional.empty();
+      var coercedArgs =
+          Streams.zip(
+                  expressions.stream(), operandTypes.stream(), FunctionConverter::coerceArgument)
+              .collect(Collectors.toList());
+      return Optional.of(generateBinding(call, matchFunction.get(), coercedArgs, outputType));
     }
 
     protected String getName() {
@@ -479,56 +454,30 @@ public interface GenericCall {
    * Coerced types according to an expected output type. Coercion is only done for type mismatches,
    * not for nullability or parameter mismatches.
    */
-  private static List<Expression> coerceArguments(List<Expression> arguments, Type type) {
-    return arguments.stream().map(a -> coerceArgument(a, type)).collect(Collectors.toList());
+  private static List<Expression> coerceArguments(List<Expression> arguments, Type targetType) {
+    return arguments.stream().map(a -> coerceArgument(a, targetType)).collect(Collectors.toList());
   }
 
   private static Expression coerceArgument(Expression argument, Type type) {
-    var typeMatches = isMatch(type, argument.getType());
-    if (!typeMatches) {
-      return ExpressionCreator.cast(type, argument, Expression.FailureBehavior.THROW_EXCEPTION);
+    if (isMatch(type, argument.getType())) {
+      return argument;
     }
-    return argument;
+
+    return ExpressionCreator.cast(type, argument, Expression.FailureBehavior.THROW_EXCEPTION);
   }
 
   protected abstract T generateBinding(
-      C call, F function, List<FunctionArg> arguments, Type outputType);
+      C call, F function, List<? extends FunctionArg> arguments, Type outputType);
 
-  public interface SingularArgumentMatcher<F> {
+  @FunctionalInterface
+  private interface SingularArgumentMatcher<F> {
     Optional<F> tryMatch(Type type, Type outputType);
   }
 
-  public interface SignatureMatcher<F> {
-    Optional<F> tryMatch(List<Type> types, Type outputType);
-  }
-
-  private static SignatureMatcher chainedSignature(SignatureMatcher... matchers) {
-    return switch (matchers.length) {
-      case 0 -> (types, outputType) -> Optional.empty();
-      case 1 -> matchers[0];
-      default -> (types, outputType) -> {
-        for (SignatureMatcher m : matchers) {
-          var t = m.tryMatch(types, outputType);
-          if (t.isPresent()) {
-            return t;
-          }
-        }
-        return Optional.empty();
-      };
-    };
-  }
-
-  private static boolean isMatch(Type inputType, ParameterizedType type) {
-    if (type.isWildcard()) {
-      return true;
-    }
-    return inputType.accept(new IgnoreNullableAndParameters(type));
-  }
-
-  private static boolean isMatch(ParameterizedType inputType, ParameterizedType type) {
-    if (type.isWildcard()) {
+  private static boolean isMatch(ParameterizedType actualType, ParameterizedType targetType) {
+    if (targetType.isWildcard()) {
       return true;
     }
-    return inputType.accept(new IgnoreNullableAndParameters(type));
+    return actualType.accept(new IgnoreNullableAndParameters(targetType));
   }
 }

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

@@ -60,7 +60,7 @@ public Optional<Expression> convert(
   protected Expression generateBinding(
       WrappedScalarCall call,
       SimpleExtension.ScalarFunctionVariant function,
-      List<FunctionArg> arguments,
+      List<? extends FunctionArg> arguments,
       Type outputType) {
     return Expression.ScalarFunctionInvocation.builder()
         .outputType(outputType)

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

@@ -52,7 +52,7 @@ public WindowFunctionConverter(
   protected Expression.WindowFunctionInvocation generateBinding(
       WrappedWindowCall call,
       SimpleExtension.WindowFunctionVariant function,
-      List<FunctionArg> arguments,
+      List<? extends FunctionArg> arguments,
       Type outputType) {
     RexOver over = call.over;
     RexWindow window = over.getWindow();

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

@@ -51,7 +51,7 @@ public WindowRelFunctionConverter(
   protected ConsistentPartitionWindow.WindowRelFunctionInvocation generateBinding(
       WrappedWindowRelCall call,
       SimpleExtension.WindowFunctionVariant function,
-      List<FunctionArg> arguments,
+      List<? extends FunctionArg> arguments,
       Type outputType) {
     Window.RexWinAggCall over = call.getWinAggCall();
 

isthmus/src/test/java/io/substrait/isthmus/ExpressionConvertabilityTest.java

@@ -65,6 +65,21 @@ public void inPredicate() throws IOException, SqlParseException {
             + "(SELECT L_SUPPKEY from LINEITEM where L_SUPPKEY < L_ORDERKEY)");
   }
 
+  @Test
+  public void concatStringLiteralAndVarchar() throws Exception {
+    assertProtoPlanRoundrip("select 'part_'||P_NAME from PART");
+  }
+
+  @Test
+  public void concatCharAndVarchar() throws Exception {
+    assertProtoPlanRoundrip("select P_BRAND||P_NAME from PART");
+  }
+
+  @Test
+  public void concatStringLiteralAndChar() throws Exception {
+    assertProtoPlanRoundrip("select 'brand_'||P_BRAND from PART");
+  }
+
   @Test
   public void singleOrList() {
     Expression singleOrList = b.singleOrList(b.fieldReference(commonTable, 0), b.i32(5), b.i32(10));

isthmus/src/test/java/io/substrait/isthmus/TpcdsQueryNoValidation.java

@@ -1,9 +1,11 @@
 package io.substrait.isthmus;
 
 import com.google.protobuf.util.JsonFormat;
+import java.util.Set;
+import java.util.stream.IntStream;
 import org.apache.calcite.adapter.tpcds.TpcdsSchema;
 import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.ValueSource;
+import org.junit.jupiter.params.provider.MethodSource;
 
 /**
  *
@@ -27,33 +29,24 @@
  */
 public class TpcdsQueryNoValidation extends PlanTestBase {
 
+  static final Set<Integer> EXCLUDED =
+      Set.of(2, 9, 12, 20, 27, 36, 47, 51, 53, 57, 63, 70, 86, 89, 98);
+
+  static IntStream testCases() {
+    return IntStream.rangeClosed(1, 99).filter(n -> !EXCLUDED.contains(n));
+  }
+
   /**
    * This test only validates that generating substrait plans for TPC-DS queries does not fail. As
    * of now this test does not validate correctness of the generated plan
    */
-  private void testQuery(int i) throws Exception {
+  @ParameterizedTest
+  @MethodSource("testCases")
+  void testQuery(int i) throws Exception {
     SqlToSubstrait s = new SqlToSubstrait();
     TpcdsSchema schema = new TpcdsSchema(1.0);
     String sql = asString(String.format("tpcds/queries/%02d.sql", i));
     var plan = s.execute(sql, "tpcds", schema);
     System.out.println(JsonFormat.printer().print(plan));
   }
-
-  @ParameterizedTest
-  @ValueSource(
-      ints = {
-        1, 3, 4, 6, 7, 8, 10, 11, 13, 14, 15, 16, 17, 18, 19, 21, 22, 23, 24, 25, 26, 28, 29, 30,
-        31, 32, 33, 34, 35, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 48, 49, 50, 52, 54, 55, 56, 58,
-        59, 60, 61, 62, 64, 65, 67, 68, 69, 71, 72, 73, 74, 75, 76, 77, 78, 79, 81, 82, 83, 85, 87,
-        88, 90, 92, 93, 94, 95, 96, 97, 99
-      })
-  public void tpcdsSuccess(int query) throws Exception {
-    testQuery(query);
-  }
-
-  @ParameterizedTest
-  @ValueSource(ints = {2, 5, 9, 12, 20, 27, 36, 47, 51, 53, 57, 63, 66, 70, 80, 84, 86, 89, 91, 98})
-  public void tpcdsFailure(int query) throws Exception {
-    // testQuery(query);
-  }
 }