Repository navigation
fix(isthmus)!: validate unary arithmetic option preferences #1395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
nielspardon
merged 10 commits into
substrait-io:main
from
alexandrefimov:unary-arithmetic-options-1173
Oct 9, 2026
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
3880d2a
fix(isthmus)!: preserve concat null handling
alexandrefimov 50d3c0b
fix(isthmus)!: preserve supported string function options
alexandrefimov 15983c3
fix(isthmus): validate string options against the selected operator
alexandrefimov c5e3352
fix(isthmus)!: validate unary arithmetic option preferences
alexandrefimov 4e90106
refactor(isthmus): share scalar option policies across both directions
alexandrefimov 2eadae2
Merge shared scalar option policies into unary arithmetic
alexandrefimov e60b285
Merge main and align unary option tests with sqrt mapping
alexandrefimov a26958a
Merge main and address arithmetic option review
alexandrefimov 278b1bd
Merge main after integer arithmetic options
alexandrefimov 2796424
Merge main after floating point arithmetic options
alexandrefimov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
160 changes: 160 additions & 0 deletions
160
isthmus/src/main/java/io/substrait/isthmus/expression/UnaryArithmeticOptions.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| package io.substrait.isthmus.expression; | ||
|
|
||
| import io.substrait.expression.Expression; | ||
| import io.substrait.expression.FunctionOption; | ||
| import io.substrait.extension.DefaultExtensionCatalog; | ||
| import io.substrait.extension.SimpleExtension.ScalarFunctionVariant; | ||
| import java.util.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. */ | ||
| final class UnaryArithmeticOptions implements ScalarFunctionOptionPolicy { | ||
| private static final Set<String> NAMES = | ||
| Set.of( | ||
| "negate", | ||
| "abs", | ||
| "sqrt", | ||
| "exp", | ||
| "cos", | ||
| "sin", | ||
| "tan", | ||
| "cosh", | ||
| "sinh", | ||
| "tanh", | ||
| "acos", | ||
| "asin", | ||
| "atan", | ||
| "acosh", | ||
| "asinh", | ||
| "atanh", | ||
| "radians", | ||
| "degrees", | ||
| "factorial"); | ||
|
|
||
| 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(); | ||
| } | ||
|
|
||
| @Override | ||
| public SqlOperator signatureOperator(RexCall call) { | ||
| return checkedIntegerNegation(call) ? SqlStdOperatorTable.UNARY_MINUS : call.getOperator(); | ||
| } | ||
|
|
||
| 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 | ||
| && SqlTypeName.INT_TYPES.contains(type); | ||
| } | ||
|
|
||
| private static FunctionOption option(String name, String value) { | ||
| return FunctionOption.builder().name(name).addValues(value).build(); | ||
| } | ||
|
|
||
| @Override | ||
| public List<FunctionOption> 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")); | ||
|
nielspardon marked this conversation as resolved.
|
||
| } | ||
| // 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), without a verified explicit option policy. | ||
| return List.of(); | ||
| } | ||
|
|
||
| @Override | ||
| public 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()) { | ||
| ScalarFunctionOptionPolicy.requireDeclaredValues(expression, option); | ||
| String optionName = option.getName().toLowerCase(Locale.ROOT); | ||
| List<String> 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)) | ||
|
nielspardon marked this conversation as resolved.
|
||
| .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; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.