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 4e901062eb966a7b063e6ccfaa1103bedca62437 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 9 Oct 2026 10:24:06 +0300 Subject: [PATCH 4/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 df6b505e3a93a1e8369be83fd0bbf26573c30579 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 9 Oct 2026 10:51:23 +0300 Subject: [PATCH 5/6] style(isthmus): brace scalar option policy conditionals --- .../isthmus/expression/ScalarFunctionConverter.java | 4 +++- .../io/substrait/isthmus/StringFunctionOptionsTest.java | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) 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 43e1590e1..ce4dc2a38 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java @@ -150,7 +150,9 @@ private Optional defaultConvert( if (finder == null) { for (ScalarFunctionOptionPolicy policy : optionPolicies) { finder = signatures.get(policy.signatureOperator(call)); - if (finder != null) break; + if (finder != null) { + break; + } } } WrappedScalarCall wrapped = new WrappedScalarCall(call); diff --git a/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java index a7079733d..c467a5091 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java @@ -249,7 +249,9 @@ void customOptionPolicyPreservesCaseInsensitiveLikeInBothDirections() { @Override public Optional getSqlOperatorFromSubstraitFunc( String key, Type outputType) { - if (key.equals("like:str_str")) return Optional.of(SqlLibraryOperators.ILIKE); + if (key.equals("like:str_str")) { + return Optional.of(SqlLibraryOperators.ILIKE); + } return super.getSqlOperatorFromSubstraitFunc(key, outputType); } @@ -272,7 +274,9 @@ protected SqlOperator resolveOptions( 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); + && call.getOperator() == SqlLibraryOperators.ILIKE) { + return List.of(insensitive); + } return super.options(call, function); } }; From f97b88eddbb9cd33d5098eb4fd9b48faf1120ecb Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 9 Oct 2026 12:42:47 +0300 Subject: [PATCH 6/6] fix(isthmus): validate declared string preferences and defer initcap charset policy --- .../expression/StringFunctionOptions.java | 26 ++++++++- .../isthmus/ConcatFunctionOptionsTest.java | 34 +++++++---- .../isthmus/StringFunctionOptionsTest.java | 56 ++++++++++++++----- 3 files changed, 88 insertions(+), 28 deletions(-) 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 6d219c554..0d0ac3c0a 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/StringFunctionOptions.java @@ -40,12 +40,14 @@ final class StringFunctionOptions implements ScalarFunctionOptionPolicy { "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))); + "initcap", new Binding("char_set", null, Set.of(SqlStdOperatorTable.INITCAP))); @Override public List forCall(RexCall call, ScalarFunctionVariant function) { Binding binding = binding(function); - if (binding == null || !binding.operators().contains(call.getOperator())) { + if (binding == null + || binding.value() == null + || !binding.operators().contains(call.getOperator())) { return List.of(); } // An omitted option lets a consumer choose any supported behavior. Emit the one @@ -69,6 +71,26 @@ public SqlOperator resolve(Expression.ScalarFunctionInvocation expression, SqlOp throw new UnsupportedOperationException( "Unsupported " + expression.declaration().name() + " option: " + option.getName()); } + for (String value : option.values()) { + boolean declared = + expression.declaration().options().entrySet().stream() + .filter(entry -> entry.getKey().equalsIgnoreCase(option.getName())) + .flatMap(entry -> entry.getValue().getValues().stream()) + .anyMatch(value::equalsIgnoreCase); + if (!declared) { + throw new UnsupportedOperationException( + expression.declaration().name() + + " option " + + option.getName() + + " does not declare value " + + value); + } + } + if (binding.value() == null) { + // The spec lists initcap's charsets without defining ASCII word boundaries. + throw new UnsupportedOperationException( + "No established Calcite initcap charset option policy"); + } // 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)) { diff --git a/isthmus/src/test/java/io/substrait/isthmus/ConcatFunctionOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/ConcatFunctionOptionsTest.java index 99621d81b..05f4a3c59 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/ConcatFunctionOptionsTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/ConcatFunctionOptionsTest.java @@ -16,9 +16,6 @@ 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; @@ -63,15 +60,11 @@ private FunctionOption nullHandling(String... preferences) { @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); + firstExpression( + "SELECT " + expression + " FROM strings", + "CREATE TABLE strings (a VARCHAR, b VARCHAR)"); assertEquals(List.of(nullHandling("ACCEPT_NULLS")), call.options()); } @@ -147,6 +140,25 @@ void rejectsAnUnknownOption() { assertTrue(failure.getMessage().contains("unknown")); } + @Test + void rejectsUndeclaredValuesEvenWithASupportedFallback() { + for (List values : + List.of(List.of("BOGUS", "ACCEPT_NULLS"), List.of("ACCEPT_NULLS", "BOGUS"))) { + UnsupportedOperationException failure = + assertThrows( + UnsupportedOperationException.class, + () -> + concat( + List.of( + FunctionOption.builder() + .name("null_handling") + .values(values) + .build())) + .accept(toRex, Context.newContext())); + assertTrue(failure.getMessage().contains("does not declare value BOGUS")); + } + } + @Test void leavesAnotherExtensionsConcatOptionsToItsMapper() { Expression.ScalarFunctionInvocation original = concat(List.of(nullHandling("IGNORE_NULLS"))); @@ -157,6 +169,6 @@ void leavesAnotherExtensionsConcatOptionsToItsMapper() { .build(); Expression.ScalarFunctionInvocation call = Expression.ScalarFunctionInvocation.builder().from(original).declaration(custom).build(); - assertEquals(call.arguments(), scalar.getExpressionArguments(call)); + call.accept(toRex, Context.newContext()); } } diff --git a/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java index c467a5091..625723964 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/StringFunctionOptionsTest.java @@ -17,8 +17,6 @@ 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; @@ -114,8 +112,7 @@ static Stream cases() { "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")); + new Case("upper", "upper(a)", "char_set", "UTF8", "ASCII_ONLY", "ÉCOLE")); } private FunctionOption option(Case c, String... values) { @@ -173,19 +170,12 @@ private Expression.ScalarFunctionInvocation call(Case c, List op @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)); + Expression.ScalarFunctionInvocation.class, + firstExpression( + "SELECT " + c.query + " FROM strings", + "CREATE TABLE strings (a VARCHAR, b VARCHAR)")); assertEquals(List.of(option(c, c.supported)), expression.options()); } @@ -217,6 +207,42 @@ void rejectsAnIncompatibleOption(Case c) { () -> call(c, List.of(option(c, c.unsupported))).accept(toRex, Context.newContext())); } + @Test + void initcapDoesNotPromiseAnUndefinedCharsetPolicy() throws Exception { + Expression.ScalarFunctionInvocation exported = + assertInstanceOf( + Expression.ScalarFunctionInvocation.class, + firstExpression("SELECT initcap(a) FROM strings", "CREATE TABLE strings (a VARCHAR)")); + assertEquals(List.of(), exported.options()); + RexNode imported = exported.accept(toRex, Context.newContext()); + assertEquals( + org.apache.calcite.sql.fun.SqlStdOperatorTable.INITCAP, + assertInstanceOf(RexCall.class, imported).getOperator()); + for (String charset : List.of("ASCII_ONLY", "UTF8")) { + Expression withOption = + Expression.ScalarFunctionInvocation.builder() + .from(exported) + .addOptions(FunctionOption.builder().name("char_set").addValues(charset).build()) + .build(); + assertThrows( + UnsupportedOperationException.class, + () -> withOption.accept(toRex, Context.newContext())); + } + } + + @ParameterizedTest + @MethodSource("cases") + void rejectsUndeclaredPreferenceValuesEvenWithASupportedFallback(Case c) { + for (List values : + List.of(List.of("BOGUS", c.supported), List.of(c.supported, "BOGUS"))) { + assertThrows( + UnsupportedOperationException.class, + () -> + call(c, List.of(FunctionOption.builder().name(c.option).values(values).build())) + .accept(toRex, Context.newContext())); + } + } + @Test void validatesOptionsAgainstTheOperatorChosenByACustomConverter() { ScalarFunctionConverter custom =