diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java index 913f4adc3..85cef0785 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java @@ -23,6 +23,8 @@ import io.substrait.isthmus.Utils; import io.substrait.isthmus.expression.FunctionMappings.Sig; import io.substrait.type.Type; +import io.substrait.type.TypeCreator; +import io.substrait.type.TypeExpressionEvaluator; import io.substrait.util.Util; import java.util.ArrayList; import java.util.Collection; @@ -47,6 +49,7 @@ import org.apache.calcite.rex.RexNode; import org.apache.calcite.sql.SqlKind; import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.type.SqlTypeUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -528,6 +531,23 @@ private Optional signatureMatch(List inputTypes, Type outputType) { return Optional.empty(); } + private List signatureMatches(List inputTypes, Type outputType) { + List matches = new ArrayList<>(); + for (F function : functions) { + List args = function.requiredArguments(); + // A variant without arguments takes no operands; signatureMatch never reaches one here + // because it stops at the first match, and the check below would index it at -1. + if (args.isEmpty() && !inputTypes.isEmpty()) { + continue; + } + if (isReturnTypeMatch(outputType, function.returnType()) + && inputTypesMatchDefinedArguments(inputTypes, args)) { + matches.add(function); + } + } + return matches; + } + private boolean isReturnTypeMatch(final Type outputType, final TypeExpression funcReturnType) { if (funcReturnType instanceof ParameterizedType) { return isMatch(outputType, (ParameterizedType) funcReturnType); @@ -817,9 +837,28 @@ private Optional matchByLeastRestrictive( Type type = typeConverter.toSubstrait(leastRestrictive); Optional out = singularInputType.orElseThrow().tryMatch(type, outputType); + List calciteTypes = + call.getOperands().map(RexNode::getType).collect(Collectors.toList()); return out.map( declaration -> { - List coercedArgs = coerceArguments(operands, type); + List coercedArgs = + Streams.zip( + operands.stream(), + calciteTypes.stream(), + (operand, calciteType) -> coerceToward(operand, calciteType, type)) + .collect(Collectors.toList()); + Optional common = losslessCommonType(operands, calciteTypes, type); + if (!binds(declaration, coercedArgs, outputType) && common.isPresent()) { + // A parameter the operands share, like the any1 of gte(any1, any1), only binds once + // they have one type, so they all take a type that holds each of them exactly. + List exact = + operands.stream() + .map(operand -> castUnlessEqual(operand, common.get())) + .collect(Collectors.toList()); + if (binds(declaration, exact, outputType)) { + coercedArgs = exact; + } + } declaration.validateOutputType(coercedArgs, outputType); return generateBinding(call, out.get(), coercedArgs, outputType); }); @@ -842,8 +881,8 @@ private Optional matchCoerced(C call, Type outputType, List expre .collect(Collectors.toList()); // See if all the input types can be made to match the function - Optional matchFunction = signatureMatch(operandTypes, outputType); - if (matchFunction.isEmpty()) { + List candidates = signatureMatches(operandTypes, outputType); + if (candidates.isEmpty()) { return Optional.empty(); } @@ -851,7 +890,23 @@ private Optional matchCoerced(C call, Type outputType, List expre Streams.zip( expressions.stream(), operandTypes.stream(), FunctionConverter::coerceArgument) .collect(Collectors.toList()); - return Optional.of(generateBinding(call, matchFunction.get(), coercedArgs, outputType)); + // The signature match takes any string for any other, so a candidate may not bind the + // operands as they are: a char(n) operand of a varchar declaration becomes a varchar(n), and + // one of a string declaration a string. The first candidate that binds is taken, and the + // first one as before when none does. + List varchars = + coercedArgs.stream() + .map(FunctionConverter::fixedCharAsVarChar) + .collect(Collectors.toList()); + for (F candidate : candidates) { + for (List arguments : + List.of(coercedArgs, varchars, asDeclaredStrings(candidate, coercedArgs))) { + if (binds(candidate, arguments, outputType)) { + return Optional.of(generateBinding(call, candidate, arguments, outputType)); + } + } + } + return Optional.of(generateBinding(call, candidates.get(0), coercedArgs, outputType)); } /** @@ -895,14 +950,159 @@ public interface GenericCall { } /** - * Coerces arguments to the target type when mismatched (ignores nullability/parameters). - * - * @param arguments input expressions - * @param targetType target Substrait type - * @return list of coerced expressions (casts applied as needed) + * Returns whether the declaration binds the given arguments: each of its parameters takes one + * value from the argument types, each argument it declares concretely has exactly that type, and + * a string result is not shorter than the call's own. concat:vchar declares one operand's length + * as its result, so {@code CHAR(2) || CHAR(2)} does not bind it: the four characters would come + * out as a varchar(2). */ - private static List coerceArguments(List arguments, Type targetType) { - return arguments.stream().map(a -> coerceArgument(a, targetType)).collect(Collectors.toList()); + private static boolean binds( + SimpleExtension.Function declaration, List arguments, Type outputType) { + List types = arguments.stream().map(Expression::getType).collect(Collectors.toList()); + try { + TypeExpressionEvaluator.checkBindings(declaration.args(), declaration.variadic(), types); + } catch (UnsupportedOperationException e) { + return false; + } + List declared = declaredArgumentTypes(declaration); + for (int index = 0; index < types.size() && !declared.isEmpty(); index++) { + ParameterizedType argument = declared.get(Math.min(index, declared.size() - 1)); + if (argument instanceof Type + && !((Type) argument).equalsIgnoringNullability(types.get(index))) { + return false; + } + } + Optional callLength = stringLength(outputType); + if (callLength.isEmpty()) { + return true; + } + try { + return stringLength(declaration.resolveType(types)) + .map(length -> length >= callLength.get()) + .orElse(true); + } catch (UnsupportedOperationException e) { + return true; + } + } + + private static List declaredArgumentTypes( + SimpleExtension.Function declaration) { + List types = new ArrayList<>(); + for (SimpleExtension.Argument argument : declaration.args()) { + if (argument instanceof SimpleExtension.ValueArgument) { + types.add(((SimpleExtension.ValueArgument) argument).value()); + } else if (argument instanceof SimpleExtension.TypeArgument) { + types.add(((SimpleExtension.TypeArgument) argument).type()); + } + } + return types; + } + + private static Optional stringLength(Type type) { + if (type instanceof Type.VarChar) { + return Optional.of(((Type.VarChar) type).length()); + } + if (type instanceof Type.FixedChar) { + return Optional.of(((Type.FixedChar) type).length()); + } + return Optional.empty(); + } + + /** + * Coerces an operand toward the least restrictive type of a call's operands. An integer operand + * of a decimal call becomes the decimal that holds every value of its type, as Calcite types it + * for decimal arithmetic, rather than the least restrictive decimal, whose scale it does not + * have: {@code decimal(7,2) * INTEGER} multiplies by a {@code decimal(10,0)}. The digits are + * Calcite's, so they follow the type system in use. + */ + private static Expression coerceToward(Expression operand, RelDataType calciteType, Type target) { + if (target instanceof Type.Decimal && SqlTypeUtil.isIntType(calciteType)) { + return ExpressionCreator.cast( + TypeCreator.of(operand.getType().nullable()).decimal(calciteType.getPrecision(), 0), + operand, + Expression.FailureBehavior.THROW_EXCEPTION); + } + return coerceArgument(operand, target); + } + + /** + * Returns a type every operand converts to without losing a value. Calcite's least restrictive + * type is that, except for decimals: past precision 38 it gives up scale, so a {@code + * decimal(38,0)} and a {@code decimal(3,2)} meet at {@code decimal(38,0)} and the second becomes + * 0. For decimals the type is built from the most integer digits and the largest scale instead, + * and there is none when that needs more than 38 digits. + */ + private static Optional losslessCommonType( + List operands, List calciteTypes, Type leastRestrictive) { + if (!(leastRestrictive instanceof Type.Decimal)) { + return Optional.of(leastRestrictive); + } + int integerDigits = 0; + int scale = 0; + for (int index = 0; index < operands.size(); index++) { + Type type = operands.get(index).getType(); + if (type instanceof Type.Decimal) { + Type.Decimal decimal = (Type.Decimal) type; + integerDigits = Math.max(integerDigits, decimal.precision() - decimal.scale()); + scale = Math.max(scale, decimal.scale()); + } else if (SqlTypeUtil.isIntType(calciteTypes.get(index))) { + integerDigits = Math.max(integerDigits, calciteTypes.get(index).getPrecision()); + } else { + return Optional.empty(); + } + } + if (integerDigits + scale > 38) { + return Optional.empty(); + } + return Optional.of( + TypeCreator.of(leastRestrictive.nullable()).decimal(integerDigits + scale, scale)); + } + + private static Expression castUnlessEqual(Expression operand, Type target) { + Type type = operand.getType(); + if (type.equalsIgnoringNullability(target)) { + return operand; + } + return ExpressionCreator.cast( + target.withNullable(type.nullable()), operand, Expression.FailureBehavior.THROW_EXCEPTION); + } + + /** + * Casts each char or varchar operand to string where the declaration takes a string there, as + * like:str_str and replace:str_str_str do. + */ + private static List asDeclaredStrings( + SimpleExtension.Function declaration, List operands) { + List declared = declaredArgumentTypes(declaration); + List arguments = new ArrayList<>(operands.size()); + for (int index = 0; index < operands.size(); index++) { + Expression operand = operands.get(index); + Type type = operand.getType(); + ParameterizedType argument = + declared.isEmpty() ? null : declared.get(Math.min(index, declared.size() - 1)); + if (argument instanceof Type.Str + && (type instanceof Type.VarChar || type instanceof Type.FixedChar)) { + arguments.add( + ExpressionCreator.cast( + TypeCreator.of(type.nullable()).STRING, + operand, + Expression.FailureBehavior.THROW_EXCEPTION)); + } else { + arguments.add(operand); + } + } + return arguments; + } + + private static Expression fixedCharAsVarChar(Expression operand) { + if (!(operand.getType() instanceof Type.FixedChar)) { + return operand; + } + Type.FixedChar fixedChar = (Type.FixedChar) operand.getType(); + return ExpressionCreator.cast( + TypeCreator.of(fixedChar.nullable()).varChar(fixedChar.length()), + operand, + Expression.FailureBehavior.THROW_EXCEPTION); } /** diff --git a/isthmus/src/test/java/io/substrait/isthmus/OperandCoercionTest.java b/isthmus/src/test/java/io/substrait/isthmus/OperandCoercionTest.java new file mode 100644 index 000000000..ceaec1a9e --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/OperandCoercionTest.java @@ -0,0 +1,137 @@ +package io.substrait.isthmus; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; + +import io.substrait.expression.Expression; +import io.substrait.isthmus.sql.SubstraitCreateStatementParser; +import io.substrait.plan.Plan; +import io.substrait.relation.Project; +import io.substrait.type.Type; +import java.util.List; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Test; + +/** + * The operands of a converted call are cast just far enough for the function's declaration to bind + * them: each of its parameters has to take one value from the argument types. + */ +class OperandCoercionTest extends PlanTestBase { + + private static final String CREATES = + "CREATE TABLE t (d7 DECIMAL(7, 2) NOT NULL, i INT NOT NULL, c CHAR(10) NOT NULL, " + + "v VARCHAR NOT NULL, v10 VARCHAR(10) NOT NULL)"; + + /** gte(any1, any1) binds any1 once, so decimals of two precisions both take the wider one. */ + @Test + void aComparisonOfTwoDecimalsCastsBothToOneType() throws Exception { + Expression.ScalarFunctionInvocation between = call("SELECT d7 BETWEEN 0.99 AND 1.49 FROM t"); + Expression.ScalarFunctionInvocation call = + assertInstanceOf(Expression.ScalarFunctionInvocation.class, between.arguments().get(0)); + + assertEquals("gte:any_any", call.declaration().key()); + assertEquals(List.of(R.decimal(7, 2), R.decimal(7, 2)), argumentTypes(call)); + assertBinds(call); + } + + /** + * multiply(decimal, decimal) binds each operand's own precision, so an integer + * operand becomes the decimal that holds it, not the least restrictive decimal(12,2). + */ + @Test + void aDecimalTimesAnIntegerCastsTheIntegerToItsOwnDecimal() throws Exception { + Expression.ScalarFunctionInvocation call = call("SELECT d7 * i FROM t"); + + assertEquals("multiply:dec_dec", call.declaration().key()); + assertEquals(List.of(R.decimal(7, 2), R.decimal(10, 0)), argumentTypes(call)); + assertBinds(call); + } + + /** A varchar declaration does not bind a char, so a char(n) operand becomes a varchar(n). */ + @Test + void aCharOperandOfAVarcharFunctionBecomesAVarchar() throws Exception { + Expression.ScalarFunctionInvocation call = call("SELECT c LIKE 'a%' FROM t"); + + assertEquals("like:vchar_vchar", call.declaration().key()); + assertEquals(R.varChar(10), argumentTypes(call).get(0)); + assertBinds(call); + } + + /** + * TPC-H declares l_discount as a bare DECIMAL, which is decimal(38,0). No decimal holds both it + * and 0.02 exactly, so the operands are left as they are rather than cast to a type that drops + * the fraction and turns the bound into 0. + */ + @Test + void noOperandIsCastToADecimalThatLosesItsScale() throws Exception { + Plan plan = + new SqlToSubstrait() + .convert( + "SELECT l_discount BETWEEN 0.03 - 0.01 AND 0.03 + 0.01 FROM lineitem", + TPCH_CATALOG); + Expression.ScalarFunctionInvocation between = + assertInstanceOf( + Expression.ScalarFunctionInvocation.class, + ((Project) plan.getRoots().get(0).getInput()).getExpressions().get(0)); + Expression.ScalarFunctionInvocation gte = + assertInstanceOf(Expression.ScalarFunctionInvocation.class, between.arguments().get(0)); + + Type.Decimal bound = assertInstanceOf(Type.Decimal.class, argumentTypes(gte).get(1)); + assertEquals(2, bound.scale()); + } + + /** + * concat:vchar declares one operand's length as its result, so two char(10) operands would come + * out as a varchar(10). The next variant, concat:str, takes them as strings. + */ + @Test + void aConcatenationNeverBindsADeclarationThatTruncatesIt() throws Exception { + Expression.ScalarFunctionInvocation call = call("SELECT c || c FROM t"); + + assertEquals("concat:str", call.declaration().key()); + assertEquals(List.of(R.STRING, R.STRING), argumentTypes(call)); + assertBinds(call); + } + + /** + * like:vchar_vchar does not bind an unbounded varchar, which is a string, so the next variant, + * like:str_str, is tried and binds once the pattern is a string too. + */ + @Test + void aVariantThatDoesNotBindGivesWayToTheNextOne() throws Exception { + Expression.ScalarFunctionInvocation call = call("SELECT v LIKE 'a%' FROM t"); + + assertEquals("like:str_str", call.declaration().key()); + assertEquals(List.of(R.STRING, R.STRING), argumentTypes(call)); + assertBinds(call); + } + + /** A declaration's concrete argument type is bound exactly: replace:str_str_str takes strings. */ + @Test + void aConcretelyDeclaredStringTakesAString() throws Exception { + Expression.ScalarFunctionInvocation call = call("SELECT REPLACE(c, v10, v10) FROM t"); + + assertEquals("replace:str_str_str", call.declaration().key()); + assertEquals(List.of(R.STRING, R.STRING, R.STRING), argumentTypes(call)); + } + + private Expression.ScalarFunctionInvocation call(String query) throws Exception { + Plan plan = + toSubstraitPlan( + query, SubstraitCreateStatementParser.processCreateStatementsToCatalog(CREATES)); + Expression expression = ((Project) plan.getRoots().get(0).getInput()).getExpressions().get(0); + return assertInstanceOf(Expression.ScalarFunctionInvocation.class, expression); + } + + private static List argumentTypes(Expression.ScalarFunctionInvocation call) { + return call.arguments().stream() + .filter(Expression.class::isInstance) + .map(argument -> ((Expression) argument).getType()) + .collect(Collectors.toList()); + } + + private static void assertBinds(Expression.ScalarFunctionInvocation call) { + assertDoesNotThrow(() -> call.declaration().resolveType(argumentTypes(call))); + } +}