From 3880d2af61e56716c4c656ab499d551bfe6e4618 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Thu, 8 Oct 2026 18:30:28 +0300 Subject: [PATCH 1/6] fix(isthmus)!: preserve concat null handling SQL CONCAT and || now emit null_handling ACCEPT_NULLS for the standard Substrait concat function. Import accepts that behavior, including as a fallback preference, instead of silently discarding options. BREAKING CHANGE: Standard concat calls with null_handling preferences that do not include ACCEPT_NULLS, unknown options, or empty preference lists are rejected. Include ACCEPT_NULLS only when null propagation is acceptable. --- .../expression/ConcatFunctionMapper.java | 33 ++++ .../expression/ScalarFunctionConverter.java | 7 +- .../isthmus/ConcatFunctionOptionsTest.java | 162 ++++++++++++++++++ 3 files changed, 201 insertions(+), 1 deletion(-) create mode 100644 isthmus/src/test/java/io/substrait/isthmus/ConcatFunctionOptionsTest.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ConcatFunctionMapper.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ConcatFunctionMapper.java index d6b3234ad..e5d795faf 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ConcatFunctionMapper.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ConcatFunctionMapper.java @@ -2,12 +2,14 @@ import io.substrait.expression.Expression; import io.substrait.expression.FunctionArg; +import io.substrait.expression.FunctionOption; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension.ScalarFunctionVariant; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; import org.apache.calcite.rex.RexCall; +import org.apache.calcite.sql.fun.SqlLibraryOperators; import org.apache.calcite.sql.fun.SqlStdOperatorTable; /** @@ -20,6 +22,23 @@ final class ConcatFunctionMapper implements ScalarFunctionMapper { private static final String CONCAT_FUNCTION_NAME = "concat"; private final List concatFunctions; + static List optionsFor(RexCall call, ScalarFunctionVariant function) { + if (!isStandardConcat(function) + || !(SqlStdOperatorTable.CONCAT.equals(call.getOperator()) + || SqlLibraryOperators.CONCAT_FUNCTION.equals(call.getOperator()))) { + return List.of(); + } + // Both Calcite operators propagate null. An omitted option lets a Substrait consumer + // choose IGNORE_NULLS instead (spec v0.103.0). + return List.of( + FunctionOption.builder().name("null_handling").addValues("ACCEPT_NULLS").build()); + } + + private static boolean isStandardConcat(ScalarFunctionVariant function) { + return CONCAT_FUNCTION_NAME.equals(function.name()) + && DefaultExtensionCatalog.FUNCTIONS_STRING.equals(function.urn()); + } + ConcatFunctionMapper(List functions) { this.concatFunctions = functions.stream() @@ -42,6 +61,20 @@ public Optional toSubstrait(RexCall call) { @Override public Optional> getExpressionArguments( Expression.ScalarFunctionInvocation expression) { + if (isStandardConcat(expression.declaration())) { + for (FunctionOption option : expression.options()) { + if (!"null_handling".equalsIgnoreCase(option.getName())) { + throw new UnsupportedOperationException("Unsupported concat option: " + option.getName()); + } + // FunctionOption preferences name acceptable behaviors, in order. Calcite supports + // only ACCEPT_NULLS, so skip other preferences and reject when none is supported. + if (option.values().stream().noneMatch("ACCEPT_NULLS"::equalsIgnoreCase)) { + throw new UnsupportedOperationException( + "Calcite concat requires null_handling ACCEPT_NULLS; preferences: " + + option.values()); + } + } + } return Optional.empty(); } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java index fa9dc7b2e..37898e2fb 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java @@ -183,7 +183,12 @@ protected Expression generateBinding( List arguments, Type outputType) { if (!DefaultExtensionCatalog.FUNCTIONS_DATETIME.equals(function.getAnchor().urn())) { - return ExpressionCreator.scalarFunction(function, outputType, arguments); + return Expression.ScalarFunctionInvocation.builder() + .declaration(function) + .outputType(outputType) + .addAllArguments(arguments) + .options(ConcatFunctionMapper.optionsFor(call.delegate, function)) + .build(); } // The datetime extension declares its results by parameter, where Calcite keeps an operand's // own type: add(date, interval_day

) is a precision_timestamp

there and a DATE here. The diff --git a/isthmus/src/test/java/io/substrait/isthmus/ConcatFunctionOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/ConcatFunctionOptionsTest.java new file mode 100644 index 000000000..99621d81b --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/ConcatFunctionOptionsTest.java @@ -0,0 +1,162 @@ +package io.substrait.isthmus; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.substrait.expression.Expression; +import io.substrait.expression.ExpressionCreator; +import io.substrait.expression.FunctionOption; +import io.substrait.extension.DefaultExtensionCatalog; +import io.substrait.extension.ImmutableSimpleExtension; +import io.substrait.extension.SimpleExtension; +import io.substrait.isthmus.SubstraitRelNodeConverter.Context; +import io.substrait.isthmus.expression.CallConverters; +import io.substrait.isthmus.expression.ExpressionRexConverter; +import io.substrait.isthmus.expression.RexExpressionConverter; +import io.substrait.isthmus.expression.ScalarFunctionConverter; +import io.substrait.isthmus.expression.WindowFunctionConverter; +import io.substrait.isthmus.sql.SubstraitCreateStatementParser; +import io.substrait.plan.Plan; +import io.substrait.relation.Project; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Stream; +import org.apache.calcite.DataContexts; +import org.apache.calcite.rex.RexExecutorImpl; +import org.apache.calcite.rex.RexLiteral; +import org.apache.calcite.rex.RexNode; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +class ConcatFunctionOptionsTest extends PlanTestBase { + private final ScalarFunctionConverter scalar = + new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory); + private final WindowFunctionConverter window = + new WindowFunctionConverter(extensions.windowFunctions(), typeFactory); + private final ExpressionRexConverter toRex = + new ExpressionRexConverter(typeFactory, scalar, window, TypeConverter.DEFAULT); + private final RexExpressionConverter fromRex = + new RexExpressionConverter( + null, + Stream.concat(CallConverters.defaults(TypeConverter.DEFAULT).stream(), Stream.of(scalar)) + .toList(), + window, + TypeConverter.DEFAULT); + + private Expression.ScalarFunctionInvocation concat(List options) { + Expression.ScalarFunctionInvocation call = + sb.scalarFn( + DefaultExtensionCatalog.FUNCTIONS_STRING, + "concat:str", + N.STRING, + ExpressionCreator.string(false, "a"), + ExpressionCreator.typedNull(N.STRING)); + return Expression.ScalarFunctionInvocation.builder().from(call).options(options).build(); + } + + private FunctionOption nullHandling(String... preferences) { + return FunctionOption.builder().name("null_handling").addValues(preferences).build(); + } + + @ParameterizedTest + @ValueSource(strings = {"concat(a, b)", "a || b", "concat(a, b, a)"}) + void sqlExportNamesItsNullHandling(String expression) throws Exception { + Plan plan = + new SqlToSubstrait() + .convert( + "SELECT " + expression + " FROM strings", + SubstraitCreateStatementParser.processCreateStatementsToCatalog( + "CREATE TABLE strings (a VARCHAR, b VARCHAR)")); + Expression.ScalarFunctionInvocation call = + (Expression.ScalarFunctionInvocation) + ((Project) plan.getRoots().get(0).getInput()).getExpressions().get(0); + assertEquals(List.of(nullHandling("ACCEPT_NULLS")), call.options()); + } + + @Test + void roundTripKeepsAnExplicitOption() { + Expression.ScalarFunctionInvocation original = concat(List.of(nullHandling("ACCEPT_NULLS"))); + RexNode rex = original.accept(toRex, Context.newContext()); + Expression.ScalarFunctionInvocation back = + (Expression.ScalarFunctionInvocation) rex.accept(fromRex); + assertEquals(original.options(), back.options()); + } + + @Test + void acceptNullsReturnsNull() { + RexNode accept = + concat(List.of(nullHandling("ACCEPT_NULLS"))).accept(toRex, Context.newContext()); + List reduced = new ArrayList<>(); + new RexExecutorImpl(DataContexts.EMPTY).reduce(creator.rex(), List.of(accept), reduced); + assertTrue(RexLiteral.isNullLiteral(reduced.get(0))); + } + + @ParameterizedTest + @ValueSource(strings = {"IGNORE_NULLS", "IGNORE_NULLS,ACCEPT_NULLS", "ACCEPT_NULLS,IGNORE_NULLS"}) + void selectsTheFirstSupportedPreference(String preferences) { + Expression.ScalarFunctionInvocation original = + concat(List.of(nullHandling(preferences.split(",")))); + if (preferences.equals("IGNORE_NULLS")) { + UnsupportedOperationException failure = + assertThrows( + UnsupportedOperationException.class, + () -> original.accept(toRex, Context.newContext())); + assertTrue(failure.getMessage().contains("ACCEPT_NULLS")); + } else { + RexNode rex = original.accept(toRex, Context.newContext()); + Expression.ScalarFunctionInvocation back = + (Expression.ScalarFunctionInvocation) rex.accept(fromRex); + assertEquals(List.of(nullHandling("ACCEPT_NULLS")), back.options()); + } + } + + @Test + void matchesOptionNamesAndValuesCaseInsensitively() { + FunctionOption option = + FunctionOption.builder().name("NULL_HANDLING").addValues("accept_nulls").build(); + RexNode rex = concat(List.of(option)).accept(toRex, Context.newContext()); + Expression.ScalarFunctionInvocation back = + (Expression.ScalarFunctionInvocation) rex.accept(fromRex); + assertEquals(List.of(nullHandling("ACCEPT_NULLS")), back.options()); + } + + @Test + void omittedOptionsAllowCalciteToChooseItsSupportedBehavior() { + RexNode rex = concat(List.of()).accept(toRex, Context.newContext()); + Expression.ScalarFunctionInvocation back = + (Expression.ScalarFunctionInvocation) rex.accept(fromRex); + assertEquals(List.of(nullHandling("ACCEPT_NULLS")), back.options()); + } + + @Test + void rejectsAnEmptyPreferenceList() { + assertThrows( + UnsupportedOperationException.class, + () -> concat(List.of(nullHandling())).accept(toRex, Context.newContext())); + } + + @Test + void rejectsAnUnknownOption() { + FunctionOption option = FunctionOption.builder().name("unknown").addValues("value").build(); + UnsupportedOperationException failure = + assertThrows( + UnsupportedOperationException.class, + () -> concat(List.of(option)).accept(toRex, Context.newContext())); + assertTrue(failure.getMessage().contains("unknown")); + } + + @Test + void leavesAnotherExtensionsConcatOptionsToItsMapper() { + Expression.ScalarFunctionInvocation original = concat(List.of(nullHandling("IGNORE_NULLS"))); + SimpleExtension.ScalarFunctionVariant custom = + ImmutableSimpleExtension.ScalarFunctionVariant.builder() + .from(original.declaration()) + .urn("extension:org.example:strings") + .build(); + Expression.ScalarFunctionInvocation call = + Expression.ScalarFunctionInvocation.builder().from(original).declaration(custom).build(); + assertEquals(call.arguments(), scalar.getExpressionArguments(call)); + } +} From 50d3c0b47212c94c95a222fb4bfd140e48848dbf Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Thu, 8 Oct 2026 18:41:06 +0300 Subject: [PATCH 2/6] fix(isthmus)!: preserve supported string function options Use one option policy for the standard concat, like, replace, starts_with, ends_with, strpos, substring, lower, upper and initcap mappings. Export the behavior implemented by the Calcite operator and reject import preferences that do not permit it. Other extensions and unmapped option policies are unchanged. BREAKING CHANGE: The covered standard string functions reject unknown options, empty preferences and preferences that do not include the mapped Calcite behavior. Allow that behavior only when it is acceptable for the query. --- .../expression/ConcatFunctionMapper.java | 33 --- .../expression/ScalarFunctionConverter.java | 3 +- .../expression/StringFunctionOptions.java | 113 ++++++++++ .../isthmus/StringFunctionOptionsTest.java | 212 ++++++++++++++++++ 4 files changed, 327 insertions(+), 34 deletions(-) create mode 100644 isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java create mode 100644 isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ConcatFunctionMapper.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ConcatFunctionMapper.java index e5d795faf..d6b3234ad 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ConcatFunctionMapper.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ConcatFunctionMapper.java @@ -2,14 +2,12 @@ import io.substrait.expression.Expression; import io.substrait.expression.FunctionArg; -import io.substrait.expression.FunctionOption; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension.ScalarFunctionVariant; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; import org.apache.calcite.rex.RexCall; -import org.apache.calcite.sql.fun.SqlLibraryOperators; import org.apache.calcite.sql.fun.SqlStdOperatorTable; /** @@ -22,23 +20,6 @@ final class ConcatFunctionMapper implements ScalarFunctionMapper { private static final String CONCAT_FUNCTION_NAME = "concat"; private final List concatFunctions; - static List optionsFor(RexCall call, ScalarFunctionVariant function) { - if (!isStandardConcat(function) - || !(SqlStdOperatorTable.CONCAT.equals(call.getOperator()) - || SqlLibraryOperators.CONCAT_FUNCTION.equals(call.getOperator()))) { - return List.of(); - } - // Both Calcite operators propagate null. An omitted option lets a Substrait consumer - // choose IGNORE_NULLS instead (spec v0.103.0). - return List.of( - FunctionOption.builder().name("null_handling").addValues("ACCEPT_NULLS").build()); - } - - private static boolean isStandardConcat(ScalarFunctionVariant function) { - return CONCAT_FUNCTION_NAME.equals(function.name()) - && DefaultExtensionCatalog.FUNCTIONS_STRING.equals(function.urn()); - } - ConcatFunctionMapper(List functions) { this.concatFunctions = functions.stream() @@ -61,20 +42,6 @@ public Optional toSubstrait(RexCall call) { @Override public Optional> getExpressionArguments( Expression.ScalarFunctionInvocation expression) { - if (isStandardConcat(expression.declaration())) { - for (FunctionOption option : expression.options()) { - if (!"null_handling".equalsIgnoreCase(option.getName())) { - throw new UnsupportedOperationException("Unsupported concat option: " + option.getName()); - } - // FunctionOption preferences name acceptable behaviors, in order. Calcite supports - // only ACCEPT_NULLS, so skip other preferences and reject when none is supported. - if (option.values().stream().noneMatch("ACCEPT_NULLS"::equalsIgnoreCase)) { - throw new UnsupportedOperationException( - "Calcite concat requires null_handling ACCEPT_NULLS; preferences: " - + option.values()); - } - } - } return Optional.empty(); } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java index 37898e2fb..85aa91346 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java @@ -187,7 +187,7 @@ protected Expression generateBinding( .declaration(function) .outputType(outputType) .addAllArguments(arguments) - .options(ConcatFunctionMapper.optionsFor(call.delegate, function)) + .options(StringFunctionOptions.forCall(call.delegate, function)) .build(); } // The datetime extension declares its results by parameter, where Calcite keeps an operand's @@ -376,6 +376,7 @@ private static Type withPrecision(Type type, int precision) { * @return the argument list, possibly remapped; never {@code null} */ public List getExpressionArguments(Expression.ScalarFunctionInvocation expression) { + StringFunctionOptions.validate(expression); // If a mapping applies to this expression, use it to get the arguments; otherwise default // behavior. return getMappedExpressionArguments(expression).orElseGet(expression::arguments); diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java b/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java new file mode 100644 index 000000000..5da0a0a6b --- /dev/null +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java @@ -0,0 +1,113 @@ +package io.substrait.isthmus.expression; + +import io.substrait.expression.Expression; +import io.substrait.expression.FunctionOption; +import io.substrait.extension.DefaultExtensionCatalog; +import io.substrait.extension.SimpleExtension.ScalarFunctionVariant; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.fun.SqlLibraryOperators; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; + +/** The spec v0.103.0 string options supported by the corresponding Calcite operators. */ +final class StringFunctionOptions { + private static final Map BINDINGS = + Map.of( + "concat", + new Binding( + "null_handling", + "ACCEPT_NULLS", + Set.of(SqlStdOperatorTable.CONCAT, SqlLibraryOperators.CONCAT_FUNCTION)), + "like", + new Binding("case_sensitivity", "CASE_SENSITIVE", Set.of(SqlStdOperatorTable.LIKE)), + "replace", + new Binding( + "case_sensitivity", "CASE_SENSITIVE", Set.of(SqlStdOperatorTable.REPLACE)), + "starts_with", + new Binding( + "case_sensitivity", "CASE_SENSITIVE", Set.of(SqlLibraryOperators.STARTS_WITH)), + "ends_with", + new Binding( + "case_sensitivity", "CASE_SENSITIVE", Set.of(SqlLibraryOperators.ENDS_WITH)), + "strpos", + new Binding( + "case_sensitivity", "CASE_SENSITIVE", Set.of(SqlStdOperatorTable.POSITION)), + "substring", + new Binding( + "negative_start", "LEFT_OF_BEGINNING", Set.of(SqlStdOperatorTable.SUBSTRING)), + "lower", new Binding("char_set", "UTF8", Set.of(SqlStdOperatorTable.LOWER)), + "upper", new Binding("char_set", "UTF8", Set.of(SqlStdOperatorTable.UPPER)), + "initcap", new Binding("char_set", "ASCII_ONLY", Set.of(SqlStdOperatorTable.INITCAP))); + + private StringFunctionOptions() {} + + static List forCall(RexCall call, ScalarFunctionVariant function) { + Binding binding = binding(function); + if (binding == null || !binding.operators().contains(call.getOperator())) { + return List.of(); + } + // An omitted option lets a consumer choose any supported behavior. Emit the one + // the producer's Calcite operator actually implements instead of a YAML default. + return List.of( + FunctionOption.builder().name(binding.name()).addValues(binding.value()).build()); + } + + static void validate(Expression.ScalarFunctionInvocation expression) { + Binding binding = binding(expression.declaration()); + if (binding == null) { + return; + } + for (FunctionOption option : expression.options()) { + if (!binding.name().equalsIgnoreCase(option.getName())) { + throw new UnsupportedOperationException( + "Unsupported " + expression.declaration().name() + " option: " + option.getName()); + } + // Preferences name acceptable behaviors, in order. These operators implement one + // behavior each, so skip other preferences and reject when none is supported. + if (option.values().stream().noneMatch(binding.value()::equalsIgnoreCase)) { + throw new UnsupportedOperationException( + "Calcite " + + expression.declaration().name() + + " requires " + + binding.name() + + " " + + binding.value() + + "; preferences: " + + option.values()); + } + } + } + + private static Binding binding(ScalarFunctionVariant function) { + return DefaultExtensionCatalog.FUNCTIONS_STRING.equals(function.urn()) + ? BINDINGS.get(function.name()) + : null; + } + + private static final class Binding { + private final String name; + private final String value; + private final Set operators; + + private Binding(String name, String value, Set operators) { + this.name = name; + this.value = value; + this.operators = operators; + } + + private String name() { + return name; + } + + private String value() { + return value; + } + + private Set operators() { + return operators; + } + } +} diff --git a/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java new file mode 100644 index 000000000..93360b5f8 --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java @@ -0,0 +1,212 @@ +package io.substrait.isthmus; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import io.substrait.expression.Expression; +import io.substrait.expression.ExpressionCreator; +import io.substrait.expression.FunctionArg; +import io.substrait.expression.FunctionOption; +import io.substrait.extension.DefaultExtensionCatalog; +import io.substrait.isthmus.SubstraitRelNodeConverter.Context; +import io.substrait.isthmus.expression.CallConverters; +import io.substrait.isthmus.expression.ExpressionRexConverter; +import io.substrait.isthmus.expression.RexExpressionConverter; +import io.substrait.isthmus.expression.ScalarFunctionConverter; +import io.substrait.isthmus.expression.WindowFunctionConverter; +import io.substrait.isthmus.sql.SubstraitCreateStatementParser; +import io.substrait.relation.Project; +import io.substrait.type.Type; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Stream; +import org.apache.calcite.DataContexts; +import org.apache.calcite.rex.RexExecutorImpl; +import org.apache.calcite.rex.RexLiteral; +import org.apache.calcite.rex.RexNode; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +class StringFunctionOptionsTest extends PlanTestBase { + private final ScalarFunctionConverter scalar = + new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory); + private final WindowFunctionConverter window = + new WindowFunctionConverter(extensions.windowFunctions(), typeFactory); + private final ExpressionRexConverter toRex = + new ExpressionRexConverter(typeFactory, scalar, window, TypeConverter.DEFAULT); + private final RexExpressionConverter fromRex = + new RexExpressionConverter( + null, + Stream.concat(CallConverters.defaults(TypeConverter.DEFAULT).stream(), Stream.of(scalar)) + .toList(), + window, + TypeConverter.DEFAULT); + + private static final class Case { + private final String name; + private final String query; + private final String option; + private final String supported; + private final String unsupported; + private final String expected; + + private Case( + String name, + String query, + String option, + String supported, + String unsupported, + String expected) { + this.name = name; + this.query = query; + this.option = option; + this.supported = supported; + this.unsupported = unsupported; + this.expected = expected; + } + } + + static Stream cases() { + return Stream.of( + new Case( + "like", "a LIKE b", "case_sensitivity", "CASE_SENSITIVE", "CASE_INSENSITIVE", "false"), + new Case( + "replace", + "replace(a,b,a)", + "case_sensitivity", + "CASE_SENSITIVE", + "CASE_INSENSITIVE", + "Abc"), + new Case( + "starts_with", + "starts_with(a,b)", + "case_sensitivity", + "CASE_SENSITIVE", + "CASE_INSENSITIVE_ASCII", + "false"), + new Case( + "ends_with", + "ends_with(a,b)", + "case_sensitivity", + "CASE_SENSITIVE", + "CASE_INSENSITIVE", + "false"), + new Case( + "strpos", + "position(b IN a)", + "case_sensitivity", + "CASE_SENSITIVE", + "CASE_INSENSITIVE", + "0"), + new Case( + "substring", + "substring(a,-1,4)", + "negative_start", + "LEFT_OF_BEGINNING", + "WRAP_FROM_END", + "ab"), + new Case("lower", "lower(a)", "char_set", "UTF8", "ASCII_ONLY", "école"), + new Case("upper", "upper(a)", "char_set", "UTF8", "ASCII_ONLY", "ÉCOLE"), + new Case("initcap", "initcap(a)", "char_set", "ASCII_ONLY", "UTF8", "éCole")); + } + + private FunctionOption option(Case c, String... values) { + return FunctionOption.builder().name(c.option).addValues(values).build(); + } + + private Expression.ScalarFunctionInvocation call(Case c, List options) { + List args; + Type output; + String key; + switch (c.name) { + case "lower", "upper", "initcap" -> { + args = + List.of( + ExpressionCreator.string( + false, + c.name.equals("initcap") + ? "éCOLE" + : c.name.equals("upper") ? "école" : "ÉCOLE")); + output = R.STRING; + key = c.name + ":str"; + } + case "substring" -> { + args = + List.of( + ExpressionCreator.string(false, "abcdef"), + ExpressionCreator.i32(false, -1), + ExpressionCreator.i32(false, 4)); + output = R.STRING; + key = "substring:str_i32_i32"; + } + default -> { + args = + new ArrayList<>( + List.of( + ExpressionCreator.string(false, c.name.equals("ends_with") ? "AbC" : "Abc"), + ExpressionCreator.string( + false, + c.name.equals("like") ? "a%" : c.name.equals("ends_with") ? "c" : "a"))); + output = c.name.equals("strpos") ? R.I64 : R.BOOLEAN; + key = c.name + ":str_str"; + if (c.name.equals("replace")) { + args.add(ExpressionCreator.string(false, "x")); + output = R.STRING; + key += "_str"; + } + } + } + return Expression.ScalarFunctionInvocation.builder() + .from(sb.scalarFn(DefaultExtensionCatalog.FUNCTIONS_STRING, key, output, args, List.of())) + .options(options) + .build(); + } + + @ParameterizedTest + @MethodSource("cases") + void exportPinsTheOperatorsBehavior(Case c) throws Exception { + Project project = + (Project) + new SqlToSubstrait() + .convert( + "SELECT " + c.query + " FROM strings", + SubstraitCreateStatementParser.processCreateStatementsToCatalog( + "CREATE TABLE strings (a VARCHAR, b VARCHAR)")) + .getRoots() + .get(0) + .getInput(); + Expression.ScalarFunctionInvocation expression = + assertInstanceOf( + Expression.ScalarFunctionInvocation.class, project.getExpressions().get(0)); + assertEquals(List.of(option(c, c.supported)), expression.options()); + } + + @ParameterizedTest + @MethodSource("cases") + void importUsesTheSupportedPreferenceAndPreservesItOnExport(Case c) { + RexNode rex = + call(c, List.of(option(c, c.unsupported, c.supported))).accept(toRex, Context.newContext()); + Expression.ScalarFunctionInvocation back = + assertInstanceOf(Expression.ScalarFunctionInvocation.class, rex.accept(fromRex)); + assertEquals(List.of(option(c, c.supported)), back.options()); + List reduced = new ArrayList<>(); + new RexExecutorImpl(DataContexts.EMPTY).reduce(creator.rex(), List.of(rex), reduced); + RexLiteral literal = assertInstanceOf(RexLiteral.class, reduced.get(0)); + String result = + c.name.equals("strpos") + ? literal.getValueAs(Long.class).toString() + : c.expected.equals("false") + ? literal.getValueAs(Boolean.class).toString() + : literal.getValueAs(String.class); + assertEquals(c.expected, result); + } + + @ParameterizedTest + @MethodSource("cases") + void rejectsAnIncompatibleOption(Case c) { + assertThrows( + UnsupportedOperationException.class, + () -> call(c, List.of(option(c, c.unsupported))).accept(toRex, Context.newContext())); + } +} From 15983c38400357673ed1d8b413c8e51938819858 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Thu, 8 Oct 2026 18:44:37 +0300 Subject: [PATCH 3/6] fix(isthmus): validate string options against the selected operator --- .../expression/ExpressionRexConverter.java | 1 + .../expression/ScalarFunctionConverter.java | 17 +++++++++++++- .../expression/StringFunctionOptions.java | 6 ++++- .../isthmus/StringFunctionOptionsTest.java | 23 +++++++++++++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java index 26f5be696..3fade7481 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java @@ -599,6 +599,7 @@ public RexNode visit(Expression.ScalarFunctionInvocation expr, Context context) callConversionFailureMessage( "scalar", expr.declaration().name(), expr.arguments()))); + scalarFunctionConverter.validateOptions(expr, operator); List eArgs = scalarFunctionConverter.getExpressionArguments(expr); List args = IntStream.range(0, eArgs.size()) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java index 85aa91346..48063f28b 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java @@ -24,6 +24,7 @@ import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rex.RexCall; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.type.SqlTypeName; /** @@ -376,12 +377,26 @@ private static Type withPrecision(Type type, int precision) { * @return the argument list, possibly remapped; never {@code null} */ public List getExpressionArguments(Expression.ScalarFunctionInvocation expression) { - StringFunctionOptions.validate(expression); // If a mapping applies to this expression, use it to get the arguments; otherwise default // behavior. return getMappedExpressionArguments(expression).orElseGet(expression::arguments); } + /** + * Checks that the selected Calcite operator can honor the invocation's options. + * + *

Custom converters selecting other operators can override this policy when they implement the + * corresponding option semantics. + * + * @param expression the Substrait invocation + * @param operator the selected Calcite operator + * @throws UnsupportedOperationException if the selected operator cannot honor the options + */ + public void validateOptions( + Expression.ScalarFunctionInvocation expression, SqlOperator operator) { + StringFunctionOptions.validate(expression, operator); + } + private Optional> getMappedExpressionArguments( Expression.ScalarFunctionInvocation expression) { return mappers.stream() diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java b/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java index 5da0a0a6b..e87d5f67c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java @@ -55,11 +55,15 @@ static List forCall(RexCall call, ScalarFunctionVariant function FunctionOption.builder().name(binding.name()).addValues(binding.value()).build()); } - static void validate(Expression.ScalarFunctionInvocation expression) { + static void validate(Expression.ScalarFunctionInvocation expression, SqlOperator operator) { Binding binding = binding(expression.declaration()); if (binding == null) { return; } + if (!expression.options().isEmpty() && !binding.operators().contains(operator)) { + throw new UnsupportedOperationException( + "No string option policy for Calcite operator " + operator.getName()); + } for (FunctionOption option : expression.options()) { if (!binding.name().equalsIgnoreCase(option.getName())) { throw new UnsupportedOperationException( diff --git a/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java index 93360b5f8..f090f5faf 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java @@ -20,11 +20,15 @@ import io.substrait.type.Type; import java.util.ArrayList; import java.util.List; +import java.util.Optional; import java.util.stream.Stream; import org.apache.calcite.DataContexts; import org.apache.calcite.rex.RexExecutorImpl; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.fun.SqlLibraryOperators; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; @@ -209,4 +213,23 @@ void rejectsAnIncompatibleOption(Case c) { UnsupportedOperationException.class, () -> call(c, List.of(option(c, c.unsupported))).accept(toRex, Context.newContext())); } + + @Test + void validatesOptionsAgainstTheOperatorChosenByACustomConverter() { + ScalarFunctionConverter custom = + new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory) { + @Override + public Optional getSqlOperatorFromSubstraitFunc( + String key, Type outputType) { + return Optional.of(SqlLibraryOperators.ILIKE); + } + }; + ExpressionRexConverter converter = + new ExpressionRexConverter(typeFactory, custom, window, TypeConverter.DEFAULT); + Case c = cases().filter(sample -> sample.name.equals("like")).findFirst().orElseThrow(); + assertThrows( + UnsupportedOperationException.class, + () -> + call(c, List.of(option(c, "CASE_SENSITIVE"))).accept(converter, Context.newContext())); + } } From b98bcb965ea12f5df9af2bfec4a74b4443dd6b63 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Thu, 8 Oct 2026 19:46:46 +0300 Subject: [PATCH 4/6] fix(isthmus)!: honor signed integer arithmetic options Select supported signed integer arithmetic preferences from spec v0.103.0, use checked Calcite operators for overflow=ERROR, and export the native operator behavior. Keep the existing operator when options are omitted. BREAKING CHANGE: Isthmus rejects signed integer arithmetic option preferences that it cannot honor instead of silently ignoring them. --- .../expression/ExpressionRexConverter.java | 2 +- .../expression/IntegerFunctionOptions.java | 160 +++++++++ .../expression/ScalarFunctionConverter.java | 24 +- .../substrait/isthmus/DdlRoundtripTest.java | 4 +- .../isthmus/IntegerArithmeticOptionsTest.java | 310 ++++++++++++++++++ .../isthmus/LambdaExpressionTest.java | 4 +- .../isthmus/NestedExpressionsTest.java | 6 +- .../io/substrait/isthmus/PlanTestBase.java | 9 + .../io/substrait/isthmus/ProjectTest.java | 5 +- .../isthmus/VirtualTableScanTest.java | 7 +- .../substrait/isthmus/VirtualTableTest.java | 8 +- .../lambdas/lambda-with-function.json | 1 + 12 files changed, 526 insertions(+), 14 deletions(-) create mode 100644 isthmus/src/main/java/io/substrait/isthmus/expression/IntegerFunctionOptions.java create mode 100644 isthmus/src/test/java/io/substrait/isthmus/IntegerArithmeticOptionsTest.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java index 26f5be696..a902e312d 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java @@ -592,7 +592,7 @@ public RexNode visit(Expression.ScalarFunctionInvocation expr, Context context) throws RuntimeException { SqlOperator operator = scalarFunctionConverter - .getSqlOperatorFromSubstraitFunc(expr.declaration().key(), expr.outputType()) + .getSqlOperatorFromSubstraitFunc(expr) .orElseThrow( () -> new IllegalArgumentException( diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/IntegerFunctionOptions.java b/isthmus/src/main/java/io/substrait/isthmus/expression/IntegerFunctionOptions.java new file mode 100644 index 000000000..5c75033ac --- /dev/null +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/IntegerFunctionOptions.java @@ -0,0 +1,160 @@ +package io.substrait.isthmus.expression; + +import io.substrait.expression.Expression; +import io.substrait.expression.FunctionOption; +import io.substrait.extension.DefaultExtensionCatalog; +import io.substrait.extension.SimpleExtension.ScalarFunctionVariant; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; + +/** Signed integer arithmetic options from spec v0.103.0 supported by Calcite. */ +final class IntegerFunctionOptions { + private IntegerFunctionOptions() {} + + static SqlOperator unchecked(SqlOperator operator) { + if (operator == SqlStdOperatorTable.CHECKED_PLUS) return SqlStdOperatorTable.PLUS; + if (operator == SqlStdOperatorTable.CHECKED_MINUS) return SqlStdOperatorTable.MINUS; + if (operator == SqlStdOperatorTable.CHECKED_MULTIPLY) return SqlStdOperatorTable.MULTIPLY; + if (operator == SqlStdOperatorTable.CHECKED_DIVIDE) return SqlStdOperatorTable.DIVIDE; + return operator; + } + + static boolean integerCall(RexCall call) { + switch (call.getType().getSqlTypeName()) { + case TINYINT: + case SMALLINT: + case INTEGER: + case BIGINT: + return call.getOperands().stream() + .allMatch(arg -> arg.getType().getSqlTypeName() == call.getType().getSqlTypeName()); + default: + return false; + } + } + + private static Binding binding(ScalarFunctionVariant function) { + if (!DefaultExtensionCatalog.FUNCTIONS_ARITHMETIC.equals(function.urn())) return null; + String name = function.name(); + for (String type : List.of("i8", "i16", "i32", "i64")) { + if (!function.key().equals(name + ":" + type + "_" + type)) continue; + boolean narrow = type.equals("i8") || type.equals("i16"); + switch (name) { + case "add": + return new Binding( + SqlStdOperatorTable.PLUS, SqlStdOperatorTable.CHECKED_PLUS, narrow, false, false); + case "subtract": + return new Binding( + SqlStdOperatorTable.MINUS, SqlStdOperatorTable.CHECKED_MINUS, narrow, false, false); + case "multiply": + return new Binding( + SqlStdOperatorTable.MULTIPLY, + SqlStdOperatorTable.CHECKED_MULTIPLY, + narrow, + false, + false); + case "divide": + return new Binding( + SqlStdOperatorTable.DIVIDE, SqlStdOperatorTable.CHECKED_DIVIDE, narrow, true, false); + case "modulus": + return new Binding(SqlStdOperatorTable.MOD, SqlStdOperatorTable.MOD, false, false, true); + default: + return null; + } + } + return null; + } + + static List forCall(RexCall call, ScalarFunctionVariant function) { + Binding binding = binding(function); + if (binding == null + || !integerCall(call) + || (call.getOperator() != binding.normal && call.getOperator() != binding.checked)) + return List.of(); + String overflow = + binding.narrow || (call.getOperator() == binding.checked && !binding.modulus) + ? "ERROR" + : "SILENT"; + List options = new ArrayList<>(); + options.add(option("overflow", overflow)); + if (binding.divide) { + options.add(option("on_domain_error", "ERROR")); + options.add(option("on_division_by_zero", "ERROR")); + } + if (binding.modulus) { + options.add(option("division_type", "TRUNCATE")); + options.add(option("on_domain_error", "ERROR")); + } + return options; + } + + private static FunctionOption option(String name, String value) { + return FunctionOption.builder().name(name).addValues(value).build(); + } + + static SqlOperator resolve(Expression.ScalarFunctionInvocation expression, SqlOperator operator) { + Binding binding = binding(expression.declaration()); + if (binding == null) return operator; + if (operator != binding.normal && operator != binding.checked) { + if (!expression.options().isEmpty()) + throw new UnsupportedOperationException( + "No integer option policy for Calcite operator " + operator.getName()); + return operator; + } + Map selected = new LinkedHashMap<>(); + for (FunctionOption option : expression.options()) { + String name = option.getName().toLowerCase(java.util.Locale.ROOT); + List supported; + if (name.equals("overflow")) { + supported = binding.narrow ? List.of("ERROR") : List.of("SILENT", "ERROR"); + } else if ((binding.divide + && (name.equals("on_domain_error") || name.equals("on_division_by_zero"))) + || (binding.modulus && name.equals("on_domain_error"))) { + supported = List.of("ERROR"); + } else if (binding.modulus && name.equals("division_type")) { + supported = List.of("TRUNCATE"); + } else { + throw new UnsupportedOperationException("Unsupported integer arithmetic option: " + name); + } + String value = + option.values().stream() + .map(v -> v.toUpperCase(java.util.Locale.ROOT)) + .filter(supported::contains) + .findFirst() + .orElseThrow( + () -> + new UnsupportedOperationException( + "Unsupported integer arithmetic " + + name + + " preferences: " + + option.values())); + String previous = selected.putIfAbsent(name, value); + if (previous != null && !previous.equals(value)) + throw new UnsupportedOperationException("Conflicting integer arithmetic option: " + name); + } + String overflow = selected.get("overflow"); + if (overflow == null) return operator; + return overflow.equals("ERROR") ? binding.checked : binding.normal; + } + + private static final class Binding { + private final SqlOperator normal; + private final SqlOperator checked; + private final boolean narrow; + private final boolean divide; + private final boolean modulus; + + private Binding( + SqlOperator normal, SqlOperator checked, boolean narrow, boolean divide, boolean modulus) { + this.normal = normal; + this.checked = checked; + this.narrow = narrow; + this.divide = divide; + this.modulus = modulus; + } + } +} diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java index fa9dc7b2e..f2f64a324 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java @@ -24,6 +24,7 @@ import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rex.RexCall; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.type.SqlTypeName; /** @@ -142,6 +143,9 @@ public Stream getOperands() { private Optional defaultConvert( RexCall call, Function topLevelConverter) { FunctionFinder finder = signatures.get(call.op); + if (finder == null && IntegerFunctionOptions.integerCall(call)) { + finder = signatures.get(IntegerFunctionOptions.unchecked(call.op)); + } WrappedScalarCall wrapped = new WrappedScalarCall(call); return attemptMatch(finder, wrapped, topLevelConverter); @@ -183,7 +187,12 @@ protected Expression generateBinding( List arguments, Type outputType) { if (!DefaultExtensionCatalog.FUNCTIONS_DATETIME.equals(function.getAnchor().urn())) { - return ExpressionCreator.scalarFunction(function, outputType, arguments); + return Expression.ScalarFunctionInvocation.builder() + .declaration(function) + .outputType(outputType) + .addAllArguments(arguments) + .options(IntegerFunctionOptions.forCall(call.delegate, function)) + .build(); } // The datetime extension declares its results by parameter, where Calcite keeps an operand's // own type: add(date, interval_day

) is a precision_timestamp

there and a DATE here. The @@ -376,6 +385,19 @@ public List getExpressionArguments(Expression.ScalarFunctionInvocat return getMappedExpressionArguments(expression).orElseGet(expression::arguments); } + /** + * Resolves the operator and the signed integer option preferences of an invocation. + * + * @param expression the Substrait scalar invocation + * @return the selected Calcite operator, or empty when no mapping exists + * @throws UnsupportedOperationException when integer options cannot be honored + */ + public Optional getSqlOperatorFromSubstraitFunc( + Expression.ScalarFunctionInvocation expression) { + return getSqlOperatorFromSubstraitFunc(expression.declaration().key(), expression.outputType()) + .map(operator -> IntegerFunctionOptions.resolve(expression, operator)); + } + private Optional> getMappedExpressionArguments( Expression.ScalarFunctionInvocation expression) { return mappers.stream() diff --git a/isthmus/src/test/java/io/substrait/isthmus/DdlRoundtripTest.java b/isthmus/src/test/java/io/substrait/isthmus/DdlRoundtripTest.java index 97ae80be1..86335917e 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/DdlRoundtripTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/DdlRoundtripTest.java @@ -314,8 +314,8 @@ private Rel computedColumns() { .input(scan) .remap(Rel.Remap.offset(2, 2)) .addExpressions( - sb.add(sb.fieldReference(scan, 0), sb.i32(1)), - sb.add(sb.fieldReference(scan, 0), sb.i32(2))) + withSilentOverflow(sb.add(sb.fieldReference(scan, 0), sb.i32(1))), + withSilentOverflow(sb.add(sb.fieldReference(scan, 0), sb.i32(2)))) .build(); } } diff --git a/isthmus/src/test/java/io/substrait/isthmus/IntegerArithmeticOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/IntegerArithmeticOptionsTest.java new file mode 100644 index 000000000..f2a140d12 --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/IntegerArithmeticOptionsTest.java @@ -0,0 +1,310 @@ +package io.substrait.isthmus; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.substrait.expression.Expression; +import io.substrait.expression.ExpressionCreator; +import io.substrait.expression.FunctionOption; +import io.substrait.extension.DefaultExtensionCatalog; +import io.substrait.isthmus.SubstraitRelNodeConverter.Context; +import io.substrait.isthmus.expression.CallConverters; +import io.substrait.isthmus.expression.ExpressionRexConverter; +import io.substrait.isthmus.expression.RexExpressionConverter; +import io.substrait.isthmus.expression.ScalarFunctionConverter; +import io.substrait.isthmus.expression.WindowFunctionConverter; +import io.substrait.isthmus.sql.SubstraitCreateStatementParser; +import io.substrait.relation.Project; +import io.substrait.type.Type; +import java.math.BigDecimal; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.util.List; +import java.util.stream.Stream; +import org.apache.calcite.DataContext; +import org.apache.calcite.jdbc.CalciteSchema; +import org.apache.calcite.linq4j.Enumerable; +import org.apache.calcite.linq4j.Linq4j; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.logical.LogicalProject; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.rex.RexLiteral; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.schema.ScannableTable; +import org.apache.calcite.schema.impl.AbstractTable; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.tools.RelBuilder; +import org.apache.calcite.tools.RelRunners; +import org.junit.jupiter.api.Test; + +class IntegerArithmeticOptionsTest extends PlanTestBase { + private final ExpressionRexConverter toRex = + new ExpressionRexConverter( + typeFactory, + new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory), + new WindowFunctionConverter(extensions.windowFunctions(), typeFactory), + TypeConverter.DEFAULT); + + private String execute(RexNode call) { + RexCall original = (RexCall) call; + CalciteSchema schema = CalciteSchema.createRootSchema(false); + schema.add("inputs", new RuntimeInputs(original)); + RelBuilder runtimeBuilder = converterProvider.getRelBuilder(schema); + RelNode input = runtimeBuilder.scan("inputs").build(); + call = + runtimeBuilder + .getRexBuilder() + .makeCall( + original.getType(), + original.getOperator(), + List.of( + runtimeBuilder.getRexBuilder().makeInputRef(input, 0), + runtimeBuilder.getRexBuilder().makeInputRef(input, 1))); + RelNode project = LogicalProject.create(input, List.of(), List.of(call), List.of("result")); + try (PreparedStatement statement = RelRunners.run(project); + ResultSet result = statement.executeQuery()) { + if (!result.next()) { + throw new IllegalStateException("No result row"); + } + return "VALUE=" + result.getObject(1); + } catch (Exception | LinkageError failure) { + Throwable root = failure; + while (root.getCause() != null) root = root.getCause(); + if (root instanceof NullPointerException) { + throw new IllegalStateException("Probe execution failed", failure); + } + return "ERROR=" + root.getClass().getSimpleName() + ": " + root.getMessage(); + } + } + + private static class RuntimeInputs extends AbstractTable implements ScannableTable { + private final RexCall call; + + private RuntimeInputs(RexCall call) { + this.call = call; + } + + @Override + public RelDataType getRowType(RelDataTypeFactory factory) { + return factory + .builder() + .add("a", call.getOperands().get(0).getType()) + .add("b", call.getOperands().get(1).getType()) + .build(); + } + + private Object value(RexNode expression) { + RexLiteral literal = (RexLiteral) expression; + switch (literal.getType().getSqlTypeName()) { + case TINYINT: + return literal.getValueAs(Integer.class).byteValue(); + case SMALLINT: + return literal.getValueAs(Integer.class).shortValue(); + case INTEGER: + return literal.getValueAs(Integer.class); + case BIGINT: + return literal.getValueAs(Long.class); + default: + return literal.getValueAs(BigDecimal.class); + } + } + + @Override + public Enumerable scan(DataContext context) { + return Linq4j.asEnumerable( + new Object[][] {{value(call.getOperands().get(0)), value(call.getOperands().get(1))}}); + } + } + + private Expression integer(int width, long value) { + switch (width) { + case 8: + return ExpressionCreator.i8(false, (byte) value); + case 16: + return ExpressionCreator.i16(false, (short) value); + case 32: + return ExpressionCreator.i32(false, (int) value); + default: + return ExpressionCreator.i64(false, value); + } + } + + private Type integerType(int width) { + return width == 8 ? R.I8 : width == 16 ? R.I16 : width == 32 ? R.I32 : R.I64; + } + + private Expression.ScalarFunctionInvocation invocation( + String name, int width, long a, long b, List options) { + return sb.scalarFn( + DefaultExtensionCatalog.FUNCTIONS_ARITHMETIC, + name + ":i" + width + "_i" + width, + integerType(width), + List.of(integer(width, a), integer(width, b)), + options); + } + + private FunctionOption option(String name, String... values) { + return FunctionOption.builder().name(name).addValues(values).build(); + } + + private Expression.ScalarFunctionInvocation export(RexNode rex) { + ScalarFunctionConverter scalar = + new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory); + RexExpressionConverter converter = + new RexExpressionConverter( + null, + Stream.concat( + CallConverters.defaults(TypeConverter.DEFAULT).stream(), Stream.of(scalar)) + .toList(), + new WindowFunctionConverter(extensions.windowFunctions(), typeFactory), + TypeConverter.DEFAULT); + return (Expression.ScalarFunctionInvocation) rex.accept(converter); + } + + @Test + void checkedOverflowIsExecutedAndSurvivesExportForEveryWidth() { + for (int width : List.of(8, 16, 32, 64)) { + long max = width == 64 ? Long.MAX_VALUE : (1L << (width - 1)) - 1; + long min = width == 64 ? Long.MIN_VALUE : -(1L << (width - 1)); + String[] names = {"add", "subtract", "multiply", "divide"}; + long[] left = {max, min, max, min}; + long[] right = {1, 1, 2, -1}; + long[] wrapped = {min, max, -2, min}; + for (int i = 0; i < names.length; i++) { + Expression.ScalarFunctionInvocation expression = + invocation(names[i], width, left[i], right[i], List.of(option("overflow", "ERROR"))); + RexNode rex = expression.accept(toRex, Context.newContext()); + assertTrue(execute(rex).startsWith("ERROR=ArithmeticException"), width + " " + names[i]); + assertTrue(export(rex).options().contains(option("overflow", "ERROR"))); + Expression.ScalarFunctionInvocation silent = + invocation(names[i], width, left[i], right[i], List.of(option("overflow", "SILENT"))); + if (width <= 16) { + assertThrows( + UnsupportedOperationException.class, + () -> silent.accept(toRex, Context.newContext())); + } else { + assertEquals("VALUE=" + wrapped[i], execute(silent.accept(toRex, Context.newContext()))); + } + } + } + } + + @Test + void preferenceOrderChoosesTheFirstSupportedBehavior() { + for (List preferences : + List.of( + List.of("ERROR", "SILENT"), List.of("SILENT", "ERROR"), List.of("SATURATE", "ERROR"))) { + RexNode rex = + invocation( + "add", + 32, + Integer.MAX_VALUE, + 1, + List.of(FunctionOption.builder().name("OVERFLOW").values(preferences).build())) + .accept(toRex, Context.newContext()); + String result = execute(rex); + if (preferences.get(0).equals("SILENT")) assertEquals("VALUE=" + Integer.MIN_VALUE, result); + else assertTrue(result.startsWith("ERROR=ArithmeticException")); + } + RexNode rex = + invocation("add", 32, 1, 2, List.of(option("overflow", "error"))) + .accept(toRex, Context.newContext()); + assertEquals("VALUE=3", execute(rex)); + assertThrows( + UnsupportedOperationException.class, + () -> + invocation("add", 32, 1, 2, List.of(option("overflow", "SATURATE"))) + .accept(toRex, Context.newContext())); + } + + @Test + void zeroDivisionAndModulusRejectNullAndAcceptErrorFallback() { + for (String name : List.of("divide", "modulus")) { + String optionName = name.equals("divide") ? "on_division_by_zero" : "on_domain_error"; + assertThrows( + UnsupportedOperationException.class, + () -> + invocation(name, 32, 1, 0, List.of(option(optionName, "NULL"))) + .accept(toRex, Context.newContext())); + RexNode rex = + invocation(name, 32, 1, 0, List.of(option(optionName, "NULL", "ERROR"))) + .accept(toRex, Context.newContext()); + assertTrue(execute(rex).startsWith("ERROR=ArithmeticException")); + } + } + + @Test + void modulusRejectsFloorAndKeepsTruncation() { + assertThrows( + UnsupportedOperationException.class, + () -> + invocation("modulus", 32, -5, 3, List.of(option("division_type", "FLOOR"))) + .accept(toRex, Context.newContext())); + RexNode rex = + invocation("modulus", 32, -5, 3, List.of(option("division_type", "FLOOR", "TRUNCATE"))) + .accept(toRex, Context.newContext()); + assertEquals("VALUE=-2", execute(rex)); + assertTrue(export(rex).options().contains(option("division_type", "TRUNCATE"))); + for (int width : List.of(8, 16, 32, 64)) { + long min = width == 64 ? Long.MIN_VALUE : -(1L << (width - 1)); + RexNode remainder = + invocation("modulus", width, min, -1, List.of(option("overflow", "ERROR"))) + .accept(toRex, Context.newContext()); + assertEquals("VALUE=0", execute(remainder)); + } + } + + @Test + void omittedOptionsKeepTheExistingOperator() { + RexCall rex = + (RexCall) + invocation("add", 32, Integer.MAX_VALUE, 1, List.of()) + .accept(toRex, Context.newContext()); + assertEquals(SqlStdOperatorTable.PLUS, rex.getOperator()); + assertEquals("VALUE=" + Integer.MIN_VALUE, execute(rex)); + } + + @Test + void unknownEmptyAndConflictingOptionsAreRejected() { + for (List options : + List.of( + List.of(option("unknown", "ERROR")), + List.of(option("overflow")), + List.of(option("overflow", "ERROR"), option("overflow", "SILENT")))) { + assertThrows( + UnsupportedOperationException.class, + () -> invocation("add", 32, 1, 2, options).accept(toRex, Context.newContext())); + } + } + + @Test + void sqlExportNamesTheActualNativeBehavior() throws Exception { + for (int width : List.of(8, 16, 32, 64)) { + String sqlType = + width == 8 ? "TINYINT" : width == 16 ? "SMALLINT" : width == 32 ? "INT" : "BIGINT"; + Project project = + (Project) + new SqlToSubstrait() + .convert( + "SELECT a+b, a-b, a*b, a/b, mod(a,b) FROM numbers", + SubstraitCreateStatementParser.processCreateStatementsToCatalog( + "CREATE TABLE numbers (a " + sqlType + ", b " + sqlType + ")")) + .getRoots() + .get(0) + .getInput(); + for (Expression expression : project.getExpressions()) { + Expression.ScalarFunctionInvocation function = + (Expression.ScalarFunctionInvocation) expression; + String overflow = + width <= 16 && !function.declaration().name().equals("modulus") ? "ERROR" : "SILENT"; + assertTrue(function.options().contains(option("overflow", overflow))); + if (function.declaration().name().equals("divide")) { + assertTrue(function.options().contains(option("on_division_by_zero", "ERROR"))); + } + } + } + } +} diff --git a/isthmus/src/test/java/io/substrait/isthmus/LambdaExpressionTest.java b/isthmus/src/test/java/io/substrait/isthmus/LambdaExpressionTest.java index 476f8a8df..cc4e174af 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/LambdaExpressionTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/LambdaExpressionTest.java @@ -81,7 +81,9 @@ void lambdaWithArithmeticBody() { Expression.Lambda lambda = lb.lambda( List.of(R.I64), - params -> sb.scalarFn(ARITH, "add:i64_i64", R.I64, params.ref(0), params.ref(0))); + params -> + withSilentOverflow( + sb.scalarFn(ARITH, "add:i64_i64", R.I64, params.ref(0), params.ref(0)))); List exprs = new ArrayList<>(); exprs.add(lambda); diff --git a/isthmus/src/test/java/io/substrait/isthmus/NestedExpressionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/NestedExpressionsTest.java index 5144c9875..8c083cbe9 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/NestedExpressionsTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/NestedExpressionsTest.java @@ -29,8 +29,10 @@ class NestedExpressionsTest extends PlanTestBase { Expression literalExpression = Expression.BoolLiteral.builder().value(true).build(); - Expression.ScalarFunctionInvocation nonLiteralExpression = sb.add(sb.i32(7), sb.i32(42)); - Expression.ScalarFunctionInvocation nonLiteralExpression2 = sb.add(sb.i32(3), sb.i32(4)); + Expression.ScalarFunctionInvocation nonLiteralExpression = + withSilentOverflow(sb.add(sb.i32(7), sb.i32(42))); + Expression.ScalarFunctionInvocation nonLiteralExpression2 = + withSilentOverflow(sb.add(sb.i32(3), sb.i32(4))); final List tableType = List.of(R.I32, R.FP32, N.STRING, N.BOOLEAN, N.STRING); final Rel commonTable = diff --git a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java index c4be9290a..d992ef80b 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java +++ b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java @@ -8,6 +8,7 @@ import com.google.common.io.Resources; import io.substrait.dsl.SubstraitBuilder; import io.substrait.expression.Expression; +import io.substrait.expression.FunctionOption; import io.substrait.extension.ExtensionCollector; import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.sql.SubstraitCreateStatementParser; @@ -88,6 +89,14 @@ protected PlanTestBase(ConverterProvider converterProvider) { this.substraitToCalcite = new SubstraitToCalcite(converterProvider); } + protected static Expression.ScalarFunctionInvocation withSilentOverflow( + Expression.ScalarFunctionInvocation expression) { + return Expression.ScalarFunctionInvocation.builder() + .from(expression) + .addOptions(FunctionOption.builder().name("overflow").addValues("SILENT").build()) + .build(); + } + public static String asString(String resource) throws IOException { return Resources.toString(Resources.getResource(resource), StandardCharsets.UTF_8); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/ProjectTest.java b/isthmus/src/test/java/io/substrait/isthmus/ProjectTest.java index b1f1c636b..c0cdb5161 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/ProjectTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/ProjectTest.java @@ -10,7 +10,10 @@ class ProjectTest extends PlanTestBase { @Test void avoidProjectRemapOnEmptyInput() { Rel projection = - Project.builder().input(emptyTable).addExpressions(sb.add(sb.i32(1), sb.i32(2))).build(); + Project.builder() + .input(emptyTable) + .addExpressions(withSilentOverflow(sb.add(sb.i32(1), sb.i32(2)))) + .build(); assertFullRoundTrip(projection); } } diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java index 84551bf38..4ae064c42 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java @@ -59,7 +59,7 @@ void expressionContainingVirtualTable() { virtualTable( schema, List.of(sb.i32(2), sb.add(sb.fp64(4.4), sb.fp64(4.5))), - List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(8.8))); + List.of(withSilentOverflow(sb.multiply(sb.i32(6), sb.i32(2))), sb.fp64(8.8))); // Check the specific Calcite encoding RelNode relNode = substraitToCalcite.convert(virtualTableScan); @@ -242,7 +242,7 @@ void structColumnWithAComputedField() { schema, List.of( ExpressionCreator.nestedStruct( - false, sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(2.0)))); + false, withSilentOverflow(sb.multiply(sb.i32(6), sb.i32(2))), sb.fp64(2.0)))); RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( @@ -414,7 +414,8 @@ void aComputedFieldInsideANullableStructConvertsBack() { schema, List.of( ExpressionCreator.nestedStruct( - true, List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(2.0))))); + true, + List.of(withSilentOverflow(sb.multiply(sb.i32(6), sb.i32(2))), sb.fp64(2.0))))); RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java index 2d9c5d9a7..5263543ca 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java @@ -46,7 +46,7 @@ private VirtualTableScan computedRows() { return virtualTable( schema, List.of(sb.i32(2), sb.add(sb.fp64(4.4), sb.fp64(4.5))), - List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(8.8))); + List.of(withSilentOverflow(sb.multiply(sb.i32(6), sb.i32(2))), sb.fp64(8.8))); } @Test @@ -74,7 +74,9 @@ void theRuleExpandsASingleRowIntoAProjection() { RelNode expanded = plan( substraitToCalcite.convert( - virtualTable(schema, List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(8.8)))), + virtualTable( + schema, + List.of(withSilentOverflow(sb.multiply(sb.i32(6), sb.i32(2))), sb.fp64(8.8)))), VirtualTableExpansionRule.instance()); assertEquals( @@ -318,7 +320,7 @@ void sqlGenerationReachesATableInsideASubquery() { VirtualTableScan oneRow = virtualTable( NamedStruct.of(List.of("col1"), R.struct(R.I32)), - List.of(sb.multiply(sb.i32(6), sb.i32(2)))); + List.of(withSilentOverflow(sb.multiply(sb.i32(6), sb.i32(2))))); Rel root = sb.project( input -> List.of(sb.scalarSubquery(oneRow, R.I32)), diff --git a/isthmus/src/test/resources/lambdas/lambda-with-function.json b/isthmus/src/test/resources/lambdas/lambda-with-function.json index 6252a2f63..3a5dbe4d8 100644 --- a/isthmus/src/test/resources/lambdas/lambda-with-function.json +++ b/isthmus/src/test/resources/lambdas/lambda-with-function.json @@ -118,6 +118,7 @@ "body": { "scalarFunction": { "functionReference": 1, + "options": [{"name": "overflow", "preference": ["SILENT"]}], "outputType": { "i32": { "nullability": "NULLABILITY_REQUIRED" From 4e901062eb966a7b063e6ccfaa1103bedca62437 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 9 Oct 2026 10:24:06 +0300 Subject: [PATCH 5/6] refactor(isthmus): share scalar option policies across both directions --- .../expression/ExpressionRexConverter.java | 3 +- .../expression/ScalarFunctionConverter.java | 52 ++++++++++++-- .../ScalarFunctionOptionPolicy.java | 20 ++++++ .../expression/StringFunctionOptions.java | 13 ++-- .../isthmus/StringFunctionOptionsTest.java | 68 +++++++++++++++++++ 5 files changed, 141 insertions(+), 15 deletions(-) create mode 100644 isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionOptionPolicy.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java index 3fade7481..a902e312d 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java @@ -592,14 +592,13 @@ public RexNode visit(Expression.ScalarFunctionInvocation expr, Context context) throws RuntimeException { SqlOperator operator = scalarFunctionConverter - .getSqlOperatorFromSubstraitFunc(expr.declaration().key(), expr.outputType()) + .getSqlOperatorFromSubstraitFunc(expr) .orElseThrow( () -> new IllegalArgumentException( callConversionFailureMessage( "scalar", expr.declaration().name(), expr.arguments()))); - scalarFunctionConverter.validateOptions(expr, operator); List eArgs = scalarFunctionConverter.getExpressionArguments(expr); List args = IntStream.range(0, eArgs.size()) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java index 48063f28b..43e1590e1 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java @@ -5,6 +5,7 @@ import io.substrait.expression.Expression; import io.substrait.expression.ExpressionCreator; import io.substrait.expression.FunctionArg; +import io.substrait.expression.FunctionOption; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.CallConverter; @@ -47,6 +48,9 @@ public class ScalarFunctionConverter */ private final List mappers; + private final List optionPolicies = + List.of(new StringFunctionOptions()); + /** * Creates a converter with the given functions and type factory. * @@ -143,6 +147,12 @@ public Stream getOperands() { private Optional defaultConvert( RexCall call, Function topLevelConverter) { FunctionFinder finder = signatures.get(call.op); + if (finder == null) { + for (ScalarFunctionOptionPolicy policy : optionPolicies) { + finder = signatures.get(policy.signatureOperator(call)); + if (finder != null) break; + } + } WrappedScalarCall wrapped = new WrappedScalarCall(call); return attemptMatch(finder, wrapped, topLevelConverter); @@ -188,7 +198,7 @@ protected Expression generateBinding( .declaration(function) .outputType(outputType) .addAllArguments(arguments) - .options(StringFunctionOptions.forCall(call.delegate, function)) + .options(options(call.delegate, function)) .build(); } // The datetime extension declares its results by parameter, where Calcite keeps an operand's @@ -383,18 +393,46 @@ public List getExpressionArguments(Expression.ScalarFunctionInvocat } /** - * Checks that the selected Calcite operator can honor the invocation's options. + * Resolves an invocation through the existing operator mapping and its option policy. * - *

Custom converters selecting other operators can override this policy when they implement the - * corresponding option semantics. + * @param expression the Substrait scalar invocation + * @return the selected operator, or empty when no mapping exists + */ + public Optional getSqlOperatorFromSubstraitFunc( + Expression.ScalarFunctionInvocation expression) { + return getSqlOperatorFromSubstraitFunc(expression.declaration().key(), expression.outputType()) + .map(operator -> resolveOptions(expression, operator)); + } + + /** + * Selects an operator that honors the requested options. Custom converters can override this + * together with {@link #options} to supply their own semantics in both directions. * * @param expression the Substrait invocation * @param operator the selected Calcite operator - * @throws UnsupportedOperationException if the selected operator cannot honor the options + * @return the operator implementing a supported preference + * @throws UnsupportedOperationException if no requested preference is supported */ - public void validateOptions( + protected SqlOperator resolveOptions( Expression.ScalarFunctionInvocation expression, SqlOperator operator) { - StringFunctionOptions.validate(expression, operator); + for (ScalarFunctionOptionPolicy policy : optionPolicies) { + operator = policy.resolve(expression, operator); + } + return operator; + } + + /** + * Returns the option preferences implemented by the matched Calcite call. + * + * @param call the Calcite call + * @param function the bound Substrait variant + * @return the preferences to export + */ + protected List options( + RexCall call, SimpleExtension.ScalarFunctionVariant function) { + return optionPolicies.stream() + .flatMap(policy -> policy.forCall(call, function).stream()) + .collect(Collectors.toList()); } private Optional> getMappedExpressionArguments( diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionOptionPolicy.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionOptionPolicy.java new file mode 100644 index 000000000..ec1ebcbd4 --- /dev/null +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionOptionPolicy.java @@ -0,0 +1,20 @@ +package io.substrait.isthmus.expression; + +import io.substrait.expression.Expression; +import io.substrait.expression.FunctionOption; +import io.substrait.extension.SimpleExtension.ScalarFunctionVariant; +import java.util.List; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.sql.SqlOperator; + +/** Option semantics for one family of scalar functions, in both conversion directions. */ +interface ScalarFunctionOptionPolicy { + SqlOperator resolve(Expression.ScalarFunctionInvocation expression, SqlOperator operator); + + List forCall(RexCall call, ScalarFunctionVariant function); + + /** Returns the operator whose signature also binds this call, such as an unchecked equivalent. */ + default SqlOperator signatureOperator(RexCall call) { + return call.getOperator(); + } +} diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java b/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java index e87d5f67c..6d219c554 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java @@ -13,7 +13,7 @@ import org.apache.calcite.sql.fun.SqlStdOperatorTable; /** The spec v0.103.0 string options supported by the corresponding Calcite operators. */ -final class StringFunctionOptions { +final class StringFunctionOptions implements ScalarFunctionOptionPolicy { private static final Map BINDINGS = Map.of( "concat", @@ -42,9 +42,8 @@ final class StringFunctionOptions { "upper", new Binding("char_set", "UTF8", Set.of(SqlStdOperatorTable.UPPER)), "initcap", new Binding("char_set", "ASCII_ONLY", Set.of(SqlStdOperatorTable.INITCAP))); - private StringFunctionOptions() {} - - static List forCall(RexCall call, ScalarFunctionVariant function) { + @Override + public List forCall(RexCall call, ScalarFunctionVariant function) { Binding binding = binding(function); if (binding == null || !binding.operators().contains(call.getOperator())) { return List.of(); @@ -55,10 +54,11 @@ static List forCall(RexCall call, ScalarFunctionVariant function FunctionOption.builder().name(binding.name()).addValues(binding.value()).build()); } - static void validate(Expression.ScalarFunctionInvocation expression, SqlOperator operator) { + @Override + public SqlOperator resolve(Expression.ScalarFunctionInvocation expression, SqlOperator operator) { Binding binding = binding(expression.declaration()); if (binding == null) { - return; + return operator; } if (!expression.options().isEmpty() && !binding.operators().contains(operator)) { throw new UnsupportedOperationException( @@ -83,6 +83,7 @@ static void validate(Expression.ScalarFunctionInvocation expression, SqlOperator + option.values()); } } + return operator; } private static Binding binding(ScalarFunctionVariant function) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java index f090f5faf..a7079733d 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java @@ -9,9 +9,11 @@ import io.substrait.expression.FunctionArg; import io.substrait.expression.FunctionOption; import io.substrait.extension.DefaultExtensionCatalog; +import io.substrait.extension.SimpleExtension.ScalarFunctionVariant; import io.substrait.isthmus.SubstraitRelNodeConverter.Context; import io.substrait.isthmus.expression.CallConverters; import io.substrait.isthmus.expression.ExpressionRexConverter; +import io.substrait.isthmus.expression.FunctionMappings; import io.substrait.isthmus.expression.RexExpressionConverter; import io.substrait.isthmus.expression.ScalarFunctionConverter; import io.substrait.isthmus.expression.WindowFunctionConverter; @@ -23,6 +25,7 @@ import java.util.Optional; import java.util.stream.Stream; import org.apache.calcite.DataContexts; +import org.apache.calcite.rex.RexCall; import org.apache.calcite.rex.RexExecutorImpl; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; @@ -232,4 +235,69 @@ public Optional getSqlOperatorFromSubstraitFunc( () -> call(c, List.of(option(c, "CASE_SENSITIVE"))).accept(converter, Context.newContext())); } + + @Test + void customOptionPolicyPreservesCaseInsensitiveLikeInBothDirections() { + Case c = cases().filter(sample -> sample.name.equals("like")).findFirst().orElseThrow(); + FunctionOption insensitive = option(c, "CASE_INSENSITIVE"); + ScalarFunctionConverter custom = + new ScalarFunctionConverter( + extensions.scalarFunctions(), + List.of(new FunctionMappings.Sig(SqlLibraryOperators.ILIKE, "like")), + typeFactory, + TypeConverter.DEFAULT) { + @Override + public Optional getSqlOperatorFromSubstraitFunc( + String key, Type outputType) { + if (key.equals("like:str_str")) return Optional.of(SqlLibraryOperators.ILIKE); + return super.getSqlOperatorFromSubstraitFunc(key, outputType); + } + + @Override + protected SqlOperator resolveOptions( + Expression.ScalarFunctionInvocation expression, SqlOperator operator) { + if (operator == SqlLibraryOperators.ILIKE + && expression.options().stream() + .allMatch( + o -> + o.getName().equalsIgnoreCase("case_sensitivity") + && o.values().stream() + .anyMatch("CASE_INSENSITIVE"::equalsIgnoreCase))) { + return operator; + } + return super.resolveOptions(expression, operator); + } + + @Override + protected List options(RexCall call, ScalarFunctionVariant function) { + if (DefaultExtensionCatalog.FUNCTIONS_STRING.equals(function.urn()) + && function.key().equals("like:str_str") + && call.getOperator() == SqlLibraryOperators.ILIKE) return List.of(insensitive); + return super.options(call, function); + } + }; + ExpressionRexConverter importer = + new ExpressionRexConverter(typeFactory, custom, window, TypeConverter.DEFAULT); + RexExpressionConverter exporter = + new RexExpressionConverter( + null, + Stream.concat( + CallConverters.defaults(TypeConverter.DEFAULT).stream(), Stream.of(custom)) + .toList(), + window, + TypeConverter.DEFAULT); + RexNode imported = call(c, List.of(insensitive)).accept(importer, Context.newContext()); + assertEquals( + SqlLibraryOperators.ILIKE, assertInstanceOf(RexCall.class, imported).getOperator()); + Expression.ScalarFunctionInvocation exported = + assertInstanceOf(Expression.ScalarFunctionInvocation.class, imported.accept(exporter)); + assertEquals(List.of(insensitive), exported.options()); + List reduced = new ArrayList<>(); + new RexExecutorImpl(DataContexts.EMPTY).reduce(creator.rex(), List.of(imported), reduced); + assertEquals( + true, assertInstanceOf(RexLiteral.class, reduced.get(0)).getValueAs(Boolean.class)); + assertThrows( + UnsupportedOperationException.class, + () -> call(c, List.of(option(c, "CASE_SENSITIVE"))).accept(importer, Context.newContext())); + } } From 667c4ee13b22b7d54e490c6bbefe49d3f332cf64 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 9 Oct 2026 11:50:28 +0300 Subject: [PATCH 6/6] fix(isthmus): retain integer options after operand promotion --- .../expression/IntegerFunctionOptions.java | 4 +- .../isthmus/IntegerArithmeticOptionsTest.java | 39 +++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/IntegerFunctionOptions.java b/isthmus/src/main/java/io/substrait/isthmus/expression/IntegerFunctionOptions.java index 4b5b6cb4c..19d821c1c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/IntegerFunctionOptions.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/IntegerFunctionOptions.java @@ -76,9 +76,9 @@ private static Binding binding(ScalarFunctionVariant function) { public List forCall(RexCall call, ScalarFunctionVariant function) { Binding binding = binding(function); if (binding == null - || !integerCall(call) - || (call.getOperator() != binding.normal && call.getOperator() != binding.checked)) + || (call.getOperator() != binding.normal && call.getOperator() != binding.checked)) { return List.of(); + } List options = new ArrayList<>(); // Plain narrow arithmetic range-checks required results but wraps nullable ones. // Planner nullability inference can change that choice, so do not promise a policy. diff --git a/isthmus/src/test/java/io/substrait/isthmus/IntegerArithmeticOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/IntegerArithmeticOptionsTest.java index de0d5599d..811cce95b 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/IntegerArithmeticOptionsTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/IntegerArithmeticOptionsTest.java @@ -352,4 +352,43 @@ void sqlExportNamesTheActualNativeBehavior() throws Exception { } } } + + @Test + void mixedIntegerWidthsExportTheBoundVariantsOptions() throws Exception { + for (String[] types : + List.of( + new String[] {"INTEGER", "SMALLINT", "i32"}, + new String[] {"BIGINT", "INTEGER", "i64"})) { + String query = "SELECT a+b, a-b, a*b, a/b, mod(a,b) FROM numbers"; + String creates = "CREATE TABLE numbers (a " + types[0] + ", b " + types[1] + ")"; + Project project = + (Project) + new SqlToSubstrait() + .convert( + query, + SubstraitCreateStatementParser.processCreateStatementsToCatalog(creates)) + .getRoots() + .get(0) + .getInput(); + for (Expression expression : project.getExpressions()) { + Expression.ScalarFunctionInvocation function = + (Expression.ScalarFunctionInvocation) expression; + assertTrue( + function.declaration().key().endsWith(":" + types[2] + "_" + types[2]), + function.declaration().key()); + assertTrue( + function.options().contains(option("overflow", "SILENT")), + function.declaration().key() + " " + function.options()); + if (function.declaration().name().equals("divide")) { + assertTrue(function.options().contains(option("on_domain_error", "ERROR"))); + assertTrue(function.options().contains(option("on_division_by_zero", "ERROR"))); + } + if (function.declaration().name().equals("modulus")) { + assertTrue(function.options().contains(option("division_type", "TRUNCATE"))); + assertTrue(function.options().contains(option("on_domain_error", "ERROR"))); + } + } + assertFullRoundTrip(query, creates); + } + } }