From 3880d2af61e56716c4c656ab499d551bfe6e4618 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Thu, 8 Oct 2026 18:30:28 +0300 Subject: [PATCH 1/5] 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/5] 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/5] 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 c5e335275c2c84f972e8176a9893f7184e4d04ba Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Thu, 8 Oct 2026 20:15:39 +0300 Subject: [PATCH 4/5] fix(isthmus)!: validate unary arithmetic option preferences Select and export supported unary arithmetic policies from spec v0.103.0: checked or silent integer negation, silent integer abs, and NAN domain errors for FP64 asin/acos. Reject explicit preferences without a proven native policy rather than dropping them. BREAKING CHANGE: Isthmus rejects unsupported unary arithmetic option preferences instead of silently ignoring them. --- .../expression/ExpressionRexConverter.java | 2 +- .../expression/ScalarFunctionConverter.java | 23 +- .../expression/UnaryArithmeticOptions.java | 136 ++++++ .../isthmus/UnaryArithmeticOptionsTest.java | 404 ++++++++++++++++++ 4 files changed, 563 insertions(+), 2 deletions(-) create mode 100644 isthmus/src/main/java/io/substrait/isthmus/expression/UnaryArithmeticOptions.java create mode 100644 isthmus/src/test/java/io/substrait/isthmus/UnaryArithmeticOptionsTest.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/ScalarFunctionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java index fa9dc7b2e..5aa701bcd 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,8 @@ public Stream getOperands() { private Optional defaultConvert( RexCall call, Function topLevelConverter) { FunctionFinder finder = signatures.get(call.op); + if (finder == null && UnaryArithmeticOptions.checkedIntegerNegation(call)) + finder = signatures.get(org.apache.calcite.sql.fun.SqlStdOperatorTable.UNARY_MINUS); WrappedScalarCall wrapped = new WrappedScalarCall(call); return attemptMatch(finder, wrapped, topLevelConverter); @@ -183,7 +186,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(UnaryArithmeticOptions.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 +384,19 @@ public List getExpressionArguments(Expression.ScalarFunctionInvocat return getMappedExpressionArguments(expression).orElseGet(expression::arguments); } + /** + * Resolves the operator and validates unary arithmetic option preferences. + * + * @param expression the Substrait scalar invocation + * @return the selected Calcite operator, or empty when no mapping exists + * @throws UnsupportedOperationException when unary arithmetic options cannot be honored + */ + public Optional getSqlOperatorFromSubstraitFunc( + Expression.ScalarFunctionInvocation expression) { + return getSqlOperatorFromSubstraitFunc(expression.declaration().key(), expression.outputType()) + .map(operator -> UnaryArithmeticOptions.resolve(expression, operator)); + } + private Optional> getMappedExpressionArguments( Expression.ScalarFunctionInvocation expression) { return mappers.stream() diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/UnaryArithmeticOptions.java b/isthmus/src/main/java/io/substrait/isthmus/expression/UnaryArithmeticOptions.java new file mode 100644 index 000000000..67f0abeb3 --- /dev/null +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/UnaryArithmeticOptions.java @@ -0,0 +1,136 @@ +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.Locale; +import java.util.Set; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.type.SqlTypeName; + +/** Unary arithmetic option policies from spec v0.103.0. */ +final class UnaryArithmeticOptions { + private static final Set NAMES = + Set.of( + "negate", + "abs", + "sqrt", + "exp", + "cos", + "sin", + "tan", + "cosh", + "sinh", + "tanh", + "acos", + "asin", + "atan", + "acosh", + "asinh", + "atanh", + "radians", + "degrees", + "factorial"); + + private UnaryArithmeticOptions() {} + + private static String name(ScalarFunctionVariant function) { + if (!DefaultExtensionCatalog.FUNCTIONS_ARITHMETIC.equals(function.urn()) + || !NAMES.contains(function.name())) return null; + for (String tag : List.of("i8", "i16", "i32", "i64", "fp32", "fp64")) + if (function.key().equals(function.name() + ":" + tag)) return function.name(); + return null; + } + + private static boolean integer(ScalarFunctionVariant function) { + return function.key().matches("(?:negate|abs):i(?:8|16|32|64)"); + } + + private static SqlOperator nativeOperator(String name) { + return FunctionMappings.SCALAR_SIGS.stream() + .filter(sig -> sig.name().equals(name)) + .map(FunctionMappings.Sig::operator) + .findFirst() + .orElseThrow(); + } + + static boolean checkedIntegerNegation(RexCall call) { + SqlTypeName type = call.getType().getSqlTypeName(); + return call.getOperator() == SqlStdOperatorTable.CHECKED_UNARY_MINUS + && call.getOperands().size() == 1 + && call.getOperands().get(0).getType().getSqlTypeName() == type + && (type == SqlTypeName.TINYINT + || type == SqlTypeName.SMALLINT + || type == SqlTypeName.INTEGER + || type == SqlTypeName.BIGINT); + } + + private static FunctionOption option(String name, String value) { + return FunctionOption.builder().name(name).addValues(value).build(); + } + + static List forCall(RexCall call, ScalarFunctionVariant function) { + String name = name(function); + if (name == null || call.getOperands().size() != 1) return List.of(); + if (name.equals("negate") && integer(function) && checkedIntegerNegation(call)) + return List.of(option("overflow", "ERROR")); + if (call.getOperator() != nativeOperator(name)) return List.of(); + if ((name.equals("negate") || name.equals("abs")) && integer(function)) + return List.of(option("overflow", "SILENT")); + // Calcite's FP32 unary conversion path cannot carry a NaN result. + if ((name.equals("acos") || name.equals("asin")) && function.key().endsWith(":fp64")) + return List.of(option("on_domain_error", "NAN")); + // Java's transcendental functions need not be correctly rounded. SQL SQRT is + // represented as POWER(x, 0.5), and native SQRT has no enumerable implementation. + return List.of(); + } + + static SqlOperator resolve(Expression.ScalarFunctionInvocation expression, SqlOperator selected) { + String name = name(expression.declaration()); + if (name == null || expression.options().isEmpty()) return selected; + SqlOperator nativeOperator = nativeOperator(name); + boolean negate = name.equals("negate") && integer(expression.declaration()); + if (selected != nativeOperator + && !(negate && selected == SqlStdOperatorTable.CHECKED_UNARY_MINUS)) + throw new UnsupportedOperationException( + "No unary arithmetic option policy for Calcite operator " + selected.getName()); + String overflow = null; + for (FunctionOption option : expression.options()) { + String optionName = option.getName().toLowerCase(Locale.ROOT); + List supported; + if (optionName.equals("overflow") && integer(expression.declaration())) + supported = negate ? List.of("SILENT", "ERROR") : List.of("SILENT"); + else if (optionName.equals("on_domain_error") + && (name.equals("acos") || name.equals("asin")) + && expression.declaration().key().endsWith(":fp64")) supported = List.of("NAN"); + else supported = List.of(); + String value = + option.values().stream() + .map(v -> v.toUpperCase(Locale.ROOT)) + .filter(supported::contains) + .findFirst() + .orElseThrow( + () -> + new UnsupportedOperationException( + "Unsupported unary arithmetic " + + name + + " " + + optionName + + " preferences: " + + option.values())); + if (optionName.equals("overflow")) { + if (overflow != null && !overflow.equals(value)) + throw new UnsupportedOperationException("Conflicting unary arithmetic option: overflow"); + overflow = value; + } + } + if (!negate || overflow == null) return selected; + return overflow.equals("ERROR") + ? SqlStdOperatorTable.CHECKED_UNARY_MINUS + : SqlStdOperatorTable.UNARY_MINUS; + } +} diff --git a/isthmus/src/test/java/io/substrait/isthmus/UnaryArithmeticOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/UnaryArithmeticOptionsTest.java new file mode 100644 index 000000000..2aa74e654 --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/UnaryArithmeticOptionsTest.java @@ -0,0 +1,404 @@ +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.sql.PreparedStatement; +import java.sql.ResultSet; +import java.util.List; +import java.util.Optional; +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.RexNode; +import org.apache.calcite.schema.ScannableTable; +import org.apache.calcite.schema.impl.AbstractTable; +import org.apache.calcite.sql.SqlOperator; +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 UnaryArithmeticOptionsTest extends PlanTestBase { + private final ExpressionRexConverter toRex = + new ExpressionRexConverter( + typeFactory, + new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory), + new WindowFunctionConverter(extensions.windowFunctions(), typeFactory), + TypeConverter.DEFAULT); + + private double execute(RexNode call, Number a) throws Exception { + RexCall original = (RexCall) call; + CalciteSchema schema = CalciteSchema.createRootSchema(false); + schema.add("inputs", new RuntimeInputs(original, a)); + 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))); + 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 result.getDouble(1); + } + } + + private static class RuntimeInputs extends AbstractTable implements ScannableTable { + private final RexCall call; + private final Object[] values; + + private RuntimeInputs(RexCall call, Number a) { + this.call = call; + Object value; + switch (call.getOperands().get(0).getType().getSqlTypeName()) { + case TINYINT: + value = a.byteValue(); + break; + case SMALLINT: + value = a.shortValue(); + break; + case INTEGER: + value = a.intValue(); + break; + case BIGINT: + value = a.longValue(); + break; + case REAL: + value = a.floatValue(); + break; + default: + value = a.doubleValue(); + } + values = new Object[] {value}; + } + + @Override + public RelDataType getRowType(RelDataTypeFactory factory) { + return factory.builder().add("a", call.getOperands().get(0).getType()).build(); + } + + @Override + public Enumerable scan(DataContext context) { + return Linq4j.asEnumerable(new Object[][] {values}); + } + } + + private Type type(String tag) { + switch (tag) { + case "i8": + return R.I8; + case "i16": + return R.I16; + case "i32": + return R.I32; + case "i64": + return R.I64; + case "fp32": + return R.FP32; + default: + return R.FP64; + } + } + + private Expression.ScalarFunctionInvocation invocation( + String name, String tag, List options) { + Expression zero; + switch (tag) { + case "i8": + zero = ExpressionCreator.i8(false, (byte) 0); + break; + case "i16": + zero = ExpressionCreator.i16(false, (short) 0); + break; + case "i32": + zero = ExpressionCreator.i32(false, 0); + break; + case "i64": + zero = ExpressionCreator.i64(false, 0); + break; + case "fp32": + zero = ExpressionCreator.fp32(false, 0); + break; + default: + zero = ExpressionCreator.fp64(false, 0); + } + return sb.scalarFn( + DefaultExtensionCatalog.FUNCTIONS_ARITHMETIC, + name + ":" + tag, + tag.equals("i64") && (name.equals("sqrt") || name.equals("exp")) ? R.FP64 : type(tag), + List.of(zero), + 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); + } + + private void assertOverflow(RexNode rex, Number value) { + Exception failure = assertThrows(Exception.class, () -> execute(rex, value)); + Throwable root = failure; + while (root.getCause() != null) root = root.getCause(); + assertTrue(root instanceof ArithmeticException, root.toString()); + } + + @Test + void negateSupportsCheckedAndSilentOverflowAtEveryWidth() throws Exception { + for (String tag : List.of("i8", "i16", "i32", "i64")) { + int width = Integer.parseInt(tag.substring(1)); + long min = width == 64 ? Long.MIN_VALUE : -(1L << (width - 1)); + RexNode checked = + invocation("negate", tag, List.of(option("overflow", "ERROR"))) + .accept(toRex, Context.newContext()); + assertOverflow(checked, min); + Expression.ScalarFunctionInvocation exported = export(checked); + assertEquals(List.of(option("overflow", "ERROR")), exported.options()); + assertOverflow(exported.accept(toRex, Context.newContext()), min); + RexNode silent = + invocation("negate", tag, List.of(option("overflow", "SILENT"))) + .accept(toRex, Context.newContext()); + assertEquals((double) min, execute(silent, min)); + assertEquals(List.of(option("overflow", "SILENT")), export(silent).options()); + assertEquals(-2.0, execute(checked, 2)); + } + } + + @Test + void preferencesSelectTheFirstSupportedOverflowMode() { + for (String tag : List.of("i8", "i16", "i32", "i64")) { + for (List preferences : + List.of( + List.of("ERROR", "SILENT"), + List.of("SILENT", "ERROR"), + List.of("SATURATE", "error"))) { + RexCall rex = + (RexCall) + invocation( + "negate", + tag, + List.of( + FunctionOption.builder().name("OVERFLOW").values(preferences).build())) + .accept(toRex, Context.newContext()); + assertEquals( + preferences.get(0).equals("SILENT") + ? SqlStdOperatorTable.UNARY_MINUS + : SqlStdOperatorTable.CHECKED_UNARY_MINUS, + rex.getOperator()); + } + assertThrows( + UnsupportedOperationException.class, + () -> + invocation("negate", tag, List.of(option("overflow", "SATURATE"))) + .accept(toRex, Context.newContext())); + } + } + + @Test + void absOnlySupportsSilentOverflow() throws Exception { + for (String tag : List.of("i8", "i16", "i32", "i64")) { + int width = Integer.parseInt(tag.substring(1)); + long min = width == 64 ? Long.MIN_VALUE : -(1L << (width - 1)); + for (String unsupported : List.of("ERROR", "SATURATE")) { + assertThrows( + UnsupportedOperationException.class, + () -> + invocation("abs", tag, List.of(option("overflow", unsupported))) + .accept(toRex, Context.newContext())); + RexNode rex = + invocation("abs", tag, List.of(option("overflow", unsupported, "SILENT"))) + .accept(toRex, Context.newContext()); + assertEquals((double) min, execute(rex, min)); + assertEquals(2.0, execute(rex, -2)); + assertEquals(List.of(option("overflow", "SILENT")), export(rex).options()); + } + } + } + + @Test + void asinAndAcosHonorNanDomainPreferences() throws Exception { + for (String name : List.of("asin", "acos")) { + for (String tag : List.of("fp64")) { + assertThrows( + UnsupportedOperationException.class, + () -> + invocation(name, tag, List.of(option("on_domain_error", "ERROR"))) + .accept(toRex, Context.newContext())); + RexNode rex = + invocation(name, tag, List.of(option("on_domain_error", "ERROR", "NAN"))) + .accept(toRex, Context.newContext()); + assertTrue(Double.isNaN(execute(rex, 2))); + assertTrue(Double.isNaN(execute(rex, Double.NaN))); + assertEquals(List.of(option("on_domain_error", "NAN")), export(rex).options()); + } + } + } + + @Test + void unprovenRoundingPreferencesAreRejectedForTheWholeUnaryFamily() { + for (String name : + List.of( + "sqrt", "exp", "cos", "sin", "tan", "cosh", "sinh", "tanh", "acos", "asin", "atan", + "acosh", "asinh", "atanh", "radians", "degrees")) { + for (String tag : List.of("fp32", "fp64")) { + for (String rounding : + List.of("TIE_TO_EVEN", "TIE_AWAY_FROM_ZERO", "TRUNCATE", "CEILING", "FLOOR")) { + assertThrows( + UnsupportedOperationException.class, + () -> + invocation(name, tag, List.of(option("rounding", rounding))) + .accept(toRex, Context.newContext()), + name + ":" + tag); + } + } + } + for (String name : List.of("sqrt", "exp")) + assertThrows( + UnsupportedOperationException.class, + () -> + invocation(name, "i64", List.of(option("rounding", "TIE_TO_EVEN"))) + .accept(toRex, Context.newContext())); + } + + @Test + void unimplementedDomainAndFactorialPoliciesAreRejected() { + for (String name : List.of("asin", "acos")) + for (String domain : List.of("NAN", "ERROR")) + assertThrows( + UnsupportedOperationException.class, + () -> + invocation(name, "fp32", List.of(option("on_domain_error", domain))) + .accept(toRex, Context.newContext())); + for (String name : List.of("sqrt", "acosh", "atanh")) + for (String domain : List.of("NAN", "ERROR")) + assertThrows( + UnsupportedOperationException.class, + () -> + invocation(name, "fp64", List.of(option("on_domain_error", domain))) + .accept(toRex, Context.newContext())); + for (String tag : List.of("i32", "i64")) + for (String overflow : List.of("SILENT", "SATURATE", "ERROR")) + assertThrows( + UnsupportedOperationException.class, + () -> + invocation("factorial", tag, List.of(option("overflow", overflow))) + .accept(toRex, Context.newContext())); + } + + @Test + void unknownEmptyAndConflictingPreferencesAreRejected() { + for (List options : + List.of( + List.of(option("overflow")), + List.of(option("unknown", "SILENT")), + List.of(option("overflow", "SILENT"), option("overflow", "ERROR")))) + assertThrows( + UnsupportedOperationException.class, + () -> invocation("negate", "i32", options).accept(toRex, Context.newContext())); + } + + @Test + void callsWithoutOptionsKeepTheExistingOperators() { + for (String name : + List.of( + "negate", "abs", "sqrt", "exp", "asin", "acos", "acosh", "atanh", "sin", "factorial")) { + String tag = + name.equals("factorial") || name.equals("negate") || name.equals("abs") ? "i64" : "fp64"; + RexCall rex = (RexCall) invocation(name, tag, List.of()).accept(toRex, Context.newContext()); + SqlOperator expected = + io.substrait.isthmus.expression.FunctionMappings.SCALAR_SIGS.stream() + .filter(sig -> sig.name().equals(name)) + .findFirst() + .orElseThrow() + .operator(); + assertEquals(expected, rex.getOperator()); + } + } + + @Test + void customOperatorsDoNotInheritNativeOptions() { + ScalarFunctionConverter custom = + new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory) { + @Override + public Optional getSqlOperatorFromSubstraitFunc( + String key, Type outputType) { + return Optional.of(SqlStdOperatorTable.UNARY_PLUS); + } + }; + assertEquals( + Optional.of(SqlStdOperatorTable.UNARY_PLUS), + custom.getSqlOperatorFromSubstraitFunc(invocation("negate", "i32", List.of()))); + assertThrows( + UnsupportedOperationException.class, + () -> + custom.getSqlOperatorFromSubstraitFunc( + invocation("negate", "i32", List.of(option("overflow", "SILENT"))))); + } + + @Test + void sqlExportOnlyNamesVerifiedNativePolicies() throws Exception { + String creates = + "CREATE TABLE numbers (i8 TINYINT, i16 SMALLINT, i32 INT, i64 BIGINT, f DOUBLE)"; + String query = + "SELECT -i8, -i16, -i32, -i64, abs(i8), abs(i16), abs(i32), abs(i64), asin(f), acos(f), sqrt(f), sin(f), exp(f) FROM numbers"; + assertFullRoundTrip(query, creates); + 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; + String name = function.declaration().name(); + if (name.equals("negate") || name.equals("abs")) + assertEquals(List.of(option("overflow", "SILENT")), function.options()); + else if (name.equals("asin") || name.equals("acos")) + assertEquals(List.of(option("on_domain_error", "NAN")), function.options()); + else assertTrue(function.options().isEmpty(), name); + } + } +} From 4e901062eb966a7b063e6ccfaa1103bedca62437 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 9 Oct 2026 10:24:06 +0300 Subject: [PATCH 5/5] 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())); + } }