From 5413db5fb71c060a3f3efcd76b15194ecdd621e8 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Thu, 20 Aug 2026 17:16:12 +0300 Subject: [PATCH 1/5] feat(core): derive return types parameterized by a length or precision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TypeExpressionEvaluator bound and evaluated two shapes, a numbered wildcard and DECIMAL, and every other parameterized class reached the throwing base. So resolveType — which FunctionBindingResolver's deriveOutputType and validateOutputType are built on — could not derive the output type of 86 declared variants across the standard catalog. The remaining classes whose parameter is an integer to substitute bind and evaluate the same way DECIMAL already did: varchar, fixedchar, fixedbinary, the three precision_* types, interval_day and interval_compound. The last two have no declared users today and are included for symmetry. list and return programs are unchanged: their parameter is a type to evaluate rather than an integer to substitute, and they need work of a different kind. --- .../type/TypeExpressionEvaluator.java | 118 ++++++++++++++++-- .../type/ParameterizedReturnTypeTest.java | 90 +++++++++++++ .../isthmus/AggregateConversion.java | 14 +-- 3 files changed, 206 insertions(+), 16 deletions(-) create mode 100644 core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java diff --git a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java index fadc4737a..b207d2b99 100644 --- a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java +++ b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java @@ -23,15 +23,21 @@ * UnsupportedOperationException}. The evaluator never falls back to a caller-supplied type: an * unresolved expression is an error, not a default. * - *

Supported shapes are concrete types, numbered wildcards ({@code any1}) and parameterized - * decimals ({@code DECIMAL}). What actually fails on the standard extension catalog is the - * other parameterized type classes — {@code varchar}, {@code fixedchar}, {@code - * precision_time

}, {@code precision_timestamp

}, {@code precision_timestamp_tz

}, {@code - * interval_day

}, {@code list}, parameterized structs — and multi-line return programs; - * {@code concat}, {@code concat_ws}, {@code assume_timezone} and the {@code strptime_*} family are - * all rejected today. Among the standard aggregates, {@code quantile} is the one whose output type - * cannot be derived at all: its declared return {@code LIST?} uses a plain {@code any}, which - * carries no identity to bind (spec v0.99.0). + *

Supported shapes are concrete types, numbered wildcards ({@code any1}), and the parameterized + * type classes whose parameter is an integer to substitute: {@code DECIMAL}, {@code + * varchar}, {@code fixedchar}, {@code fixedbinary}, {@code precision_time

}, {@code + * precision_timestamp

}, {@code precision_timestamp_tz

}, {@code interval_day

} and {@code + * interval_compound

}. The last two of those are supported for symmetry; no standard extension + * declares them parameterized today. + * + *

What still fails on the standard extension catalog is a return of {@code list}, whose + * parameter is a type to evaluate rather than an integer to substitute ({@code filter}, {@code + * sort}, {@code transform}, {@code string_split}, {@code regexp_string_split}, {@code + * regexp_match_substring_all}), and a multi-line return program ({@code add}, {@code subtract}, + * {@code multiply}, {@code divide}, {@code modulus}, {@code ceil}, {@code floor}, {@code round}, + * the {@code bitwise_*} family, {@code assume_timezone} and {@code strptime_*}). Among the standard + * aggregates, {@code quantile} cannot be derived at all: its declared return {@code LIST?} + * uses a plain {@code any}, which carries no identity to bind (spec v0.101.0). */ public class TypeExpressionEvaluator { @@ -173,6 +179,53 @@ private void bind(ParameterizedType declared, Type actual, boolean bindNames) { Type.Decimal actualDecimal = (Type.Decimal) actual; bindInteger(declaredDecimal.precision().value(), actualDecimal.precision(), bindNames); bindInteger(declaredDecimal.scale().value(), actualDecimal.scale(), bindNames); + } else if (declared instanceof ParameterizedType.FixedChar + && actual instanceof Type.FixedChar) { + bindInteger( + ((ParameterizedType.FixedChar) declared).length().value(), + ((Type.FixedChar) actual).length(), + bindNames); + } else if (declared instanceof ParameterizedType.VarChar && actual instanceof Type.VarChar) { + bindInteger( + ((ParameterizedType.VarChar) declared).length().value(), + ((Type.VarChar) actual).length(), + bindNames); + } else if (declared instanceof ParameterizedType.FixedBinary + && actual instanceof Type.FixedBinary) { + bindInteger( + ((ParameterizedType.FixedBinary) declared).length().value(), + ((Type.FixedBinary) actual).length(), + bindNames); + } else if (declared instanceof ParameterizedType.PrecisionTime + && actual instanceof Type.PrecisionTime) { + bindInteger( + ((ParameterizedType.PrecisionTime) declared).precision().value(), + ((Type.PrecisionTime) actual).precision(), + bindNames); + } else if (declared instanceof ParameterizedType.PrecisionTimestamp + && actual instanceof Type.PrecisionTimestamp) { + bindInteger( + ((ParameterizedType.PrecisionTimestamp) declared).precision().value(), + ((Type.PrecisionTimestamp) actual).precision(), + bindNames); + } else if (declared instanceof ParameterizedType.PrecisionTimestampTZ + && actual instanceof Type.PrecisionTimestampTZ) { + bindInteger( + ((ParameterizedType.PrecisionTimestampTZ) declared).precision().value(), + ((Type.PrecisionTimestampTZ) actual).precision(), + bindNames); + } else if (declared instanceof ParameterizedType.IntervalDay + && actual instanceof Type.IntervalDay) { + bindInteger( + ((ParameterizedType.IntervalDay) declared).precision().value(), + ((Type.IntervalDay) actual).precision(), + bindNames); + } else if (declared instanceof ParameterizedType.IntervalCompound + && actual instanceof Type.IntervalCompound) { + bindInteger( + ((ParameterizedType.IntervalCompound) declared).precision().value(), + ((Type.IntervalCompound) actual).precision(), + bindNames); } } @@ -245,6 +298,53 @@ public Type visit(ParameterizedType.Decimal decimal) { return TypeCreator.of(decimal.nullable()).decimal(precision, scale); } + @Override + public Type visit(ParameterizedType.FixedChar fixedChar) { + return TypeCreator.of(fixedChar.nullable()) + .fixedChar(resolveInteger(fixedChar.length().value())); + } + + @Override + public Type visit(ParameterizedType.VarChar varChar) { + return TypeCreator.of(varChar.nullable()).varChar(resolveInteger(varChar.length().value())); + } + + @Override + public Type visit(ParameterizedType.FixedBinary fixedBinary) { + return TypeCreator.of(fixedBinary.nullable()) + .fixedBinary(resolveInteger(fixedBinary.length().value())); + } + + @Override + public Type visit(ParameterizedType.PrecisionTime precisionTime) { + return TypeCreator.of(precisionTime.nullable()) + .precisionTime(resolveInteger(precisionTime.precision().value())); + } + + @Override + public Type visit(ParameterizedType.PrecisionTimestamp precisionTimestamp) { + return TypeCreator.of(precisionTimestamp.nullable()) + .precisionTimestamp(resolveInteger(precisionTimestamp.precision().value())); + } + + @Override + public Type visit(ParameterizedType.PrecisionTimestampTZ precisionTimestampTZ) { + return TypeCreator.of(precisionTimestampTZ.nullable()) + .precisionTimestampTZ(resolveInteger(precisionTimestampTZ.precision().value())); + } + + @Override + public Type visit(ParameterizedType.IntervalDay intervalDay) { + return TypeCreator.of(intervalDay.nullable()) + .intervalDay(resolveInteger(intervalDay.precision().value())); + } + + @Override + public Type visit(ParameterizedType.IntervalCompound intervalCompound) { + return TypeCreator.of(intervalCompound.nullable()) + .intervalCompound(resolveInteger(intervalCompound.precision().value())); + } + @Override public Type visit(ParameterizedType.StringLiteral stringLiteral) { // A wildcard return (e.g. min(any1) -> any1) resolves to the bound argument type, taking the diff --git a/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java new file mode 100644 index 000000000..7e3c477c5 --- /dev/null +++ b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java @@ -0,0 +1,90 @@ +package io.substrait.type; + +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.extension.DefaultExtensionCatalog; +import io.substrait.extension.SimpleExtension; +import java.util.List; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; + +/** + * Pins the return-type derivation for the parameterized shapes, against the declarations the + * standard extensions actually ship rather than against hand-written ones. + */ +class ParameterizedReturnTypeTest { + + private static final TypeCreator R = TypeCreator.REQUIRED; + private static final TypeCreator N = TypeCreator.NULLABLE; + private static final SimpleExtension.ExtensionCollection EXTENSIONS = + DefaultExtensionCatalog.DEFAULT_COLLECTION; + + private static SimpleExtension.Function variant(String key) { + return Stream.of( + EXTENSIONS.scalarFunctions(), + EXTENSIONS.aggregateFunctions(), + EXTENSIONS.windowFunctions()) + .flatMap(List::stream) + .filter(f -> f.key().equals(key)) + .findFirst() + .orElseThrow(() -> new AssertionError("no such variant: " + key)); + } + + private static Type resolve(String key, Type... args) { + return variant(key).resolveType(List.of(args)); + } + + @Test + void precisionTimestampCarriesItsPrecision() { + assertEquals( + R.precisionTimestamp(3), + resolve("add:pts_iyear", R.precisionTimestamp(3), R.INTERVAL_YEAR)); + } + + @Test + void intervalDayCarriesItsPrecision() { + assertEquals(R.intervalDay(9), resolve("multiply:i8_iday", R.I8, R.intervalDay(9))); + } + + @Test + void dateMinusIntervalDayDerivesAPrecisionTimestamp() { + // The signature #1117 is about: the spec declares precision_timestamp

, where P is the + // interval's, and nothing but the interval carries it. + assertEquals(R.precisionTimestamp(6), resolve("subtract:date_iday", R.DATE, R.intervalDay(6))); + assertEquals(R.precisionTimestamp(3), resolve("subtract:date_iday", R.DATE, R.intervalDay(3))); + } + + @Test + void oneParameterSharedByTwoArgumentsHasToAgree() { + assertEquals( + R.intervalDay(3), resolve("add_intervals:iday_iday", R.intervalDay(3), R.intervalDay(3))); + + UnsupportedOperationException e = + assertThrows( + UnsupportedOperationException.class, + () -> resolve("add_intervals:iday_iday", R.intervalDay(3), R.intervalDay(6))); + assertTrue(e.getMessage().contains("P"), e.getMessage()); + } + + @Test + void varCharAndFixedCharCarryTheirLength() { + assertEquals(R.varChar(20), resolve("concat:vchar", R.varChar(20), R.varChar(20))); + assertEquals(R.fixedChar(8), resolve("reverse:fchar", R.fixedChar(8))); + } + + @Test + void theOtherTemporalShapesCarryTheirPrecision() { + assertEquals(N.precisionTime(3), resolve("min:pt", R.precisionTime(3))); + assertEquals( + R.precisionTimestampTZ(9), + resolve("add:ptstz_iyear_str", R.precisionTimestampTZ(9), R.INTERVAL_YEAR, R.STRING)); + } + + @Test + void mirrorNullabilityStillApplies() { + // The declared return is non-null; MIRROR makes it nullable because an argument is. + assertEquals(N.intervalDay(6), resolve("multiply:i8_iday", R.I8, N.intervalDay(6))); + } +} diff --git a/isthmus/src/main/java/io/substrait/isthmus/AggregateConversion.java b/isthmus/src/main/java/io/substrait/isthmus/AggregateConversion.java index 25253969f..5a76b0a39 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/AggregateConversion.java +++ b/isthmus/src/main/java/io/substrait/isthmus/AggregateConversion.java @@ -32,14 +32,14 @@ public enum FunctionBindingValidation { * *

Type derivation is fail-closed: a function whose return expression the derivation does not * yet support is rejected rather than assumed valid, so this mode is not adoptable for plans - * that use such functions. On the standard extension catalog the unsupported shapes are the - * parameterized type classes other than decimal — {@code varchar}, {@code fixedchar}, - * {@code precision_time

}, {@code precision_timestamp

}, {@code precision_timestamp_tz

}, - * {@code interval_day

}, {@code list}, parameterized structs — and multi-line return - * programs; for example {@code concat}, {@code concat_ws}, {@code assume_timezone} and the - * {@code strptime_*} family are rejected today. {@code quantile}'s output type cannot be + * that use such functions. On the standard extension catalog what remains unsupported is a + * return of {@code list} ({@code filter}, {@code sort}, {@code transform}, {@code + * string_split}, {@code regexp_string_split}, {@code regexp_match_substring_all}) and a + * multi-line return program ({@code add}, {@code subtract}, {@code multiply}, {@code divide}, + * {@code modulus}, {@code ceil}, {@code floor}, {@code round}, the {@code bitwise_*} family, + * {@code assume_timezone} and {@code strptime_*}). {@code quantile}'s output type cannot be * derived at all: its declared return {@code LIST?} uses a plain {@code any}, which - * carries no identity to bind (spec v0.99.0). + * carries no identity to bind (spec v0.101.0). */ EXTENSION_DECLARATION } From 8f749a0fe9ee5e9fd66cc94c4749a17cc3c11c4d Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Wed, 26 Aug 2026 16:20:49 +0300 Subject: [PATCH 2/5] test(core): pin the return shapes that are not derived yet The Javadoc listed them by function name, where only the decimal variants of add, subtract and the rest carry a return program, and nothing checked the list at all: a spec bump would move it without saying so. Pin both shapes by the variants that carry them, and name the variants in the text. --- .../type/TypeExpressionEvaluator.java | 12 ++-- .../type/ParameterizedReturnTypeTest.java | 55 +++++++++++++++++++ 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java index b207d2b99..445fdd7a0 100644 --- a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java +++ b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java @@ -33,11 +33,13 @@ *

What still fails on the standard extension catalog is a return of {@code list}, whose * parameter is a type to evaluate rather than an integer to substitute ({@code filter}, {@code * sort}, {@code transform}, {@code string_split}, {@code regexp_string_split}, {@code - * regexp_match_substring_all}), and a multi-line return program ({@code add}, {@code subtract}, - * {@code multiply}, {@code divide}, {@code modulus}, {@code ceil}, {@code floor}, {@code round}, - * the {@code bitwise_*} family, {@code assume_timezone} and {@code strptime_*}). Among the standard - * aggregates, {@code quantile} cannot be derived at all: its declared return {@code LIST?} - * uses a plain {@code any}, which carries no identity to bind (spec v0.101.0). + * regexp_match_substring_all}), and a multi-line return program -- the decimal variants of {@code + * add}, {@code subtract}, {@code multiply}, {@code divide}, {@code modulus}, {@code ceil}, {@code + * floor}, {@code round} and the {@code bitwise_*} family, together with {@code + * assume_timezone:date_str_i8} and the {@code strptime_*} family. Among the standard aggregates, + * {@code quantile} cannot be derived at all: its declared return {@code LIST?} uses a plain + * {@code any}, which carries no identity to bind. Both lists are pinned against the catalog by + * {@code ParameterizedReturnTypeTest}, which is spec v0.101.0 today. */ public class TypeExpressionEvaluator { diff --git a/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java index 7e3c477c5..ad960d8db 100644 --- a/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java +++ b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java @@ -6,7 +6,10 @@ import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; +import io.substrait.function.ParameterizedType; +import io.substrait.function.TypeExpression; import java.util.List; +import java.util.stream.Collectors; import java.util.stream.Stream; import org.junit.jupiter.api.Test; @@ -87,4 +90,56 @@ void mirrorNullabilityStillApplies() { // The declared return is non-null; MIRROR makes it nullable because an argument is. assertEquals(N.intervalDay(6), resolve("multiply:i8_iday", R.I8, N.intervalDay(6))); } + + /** + * The two return shapes the evaluator does not derive, pinned by the variants that carry them so + * that the list in {@link TypeExpressionEvaluator}'s Javadoc cannot go stale on its own. A + * parameter that is a type to evaluate rather than an integer to substitute is the first; a + * multi-line return program is the second. + */ + @Test + void theReturnShapesThatAreNotDerivedYet() { + assertEquals( + List.of( + "filter:list_func", + "quantile:req_req_i64_any", + "regexp_match_substring_all:vchar_vchar_i64_i64", + "regexp_string_split:vchar_vchar", + "sort:list", + "string_split:vchar_vchar", + "transform:list_func"), + variantsReturning(ParameterizedType.ListType.class)); + + assertEquals( + List.of( + "add:dec_dec", + "assume_timezone:date_str_i8", + "bitwise_and:dec_dec", + "bitwise_or:dec_dec", + "bitwise_xor:dec_dec", + "ceil:dec", + "divide:dec_dec", + "floor:dec", + "modulus:dec_dec", + "multiply:dec_dec", + "round:dec_i32", + "strptime_time:str_str_i8", + "strptime_timestamp:str_str_i8", + "strptime_timestamp:str_str_str_i8", + "subtract:dec_dec"), + variantsReturning(TypeExpression.ReturnProgram.class)); + } + + private static List variantsReturning(Class returnShape) { + return Stream.of( + EXTENSIONS.scalarFunctions(), + EXTENSIONS.aggregateFunctions(), + EXTENSIONS.windowFunctions()) + .flatMap(List::stream) + .filter(f -> returnShape.isInstance(f.returnType())) + .map(SimpleExtension.Function::key) + .distinct() + .sorted() + .collect(Collectors.toList()); + } } From 0dd2f17203b5e95cc307c7e8d70f8c3206707a2e Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 28 Aug 2026 10:32:47 +0300 Subject: [PATCH 3/5] test(core): pin the two parameterized shapes the catalog does not declare fixedbinary and interval_compound were derived but unreachable from any standard extension, so nothing exercised them: hardcoding either resolution left the suite green. They are pinned against hand-written declarations now. The class comment said interval_day was in that position too. It is not -- nine variants return interval_day

, and the test that resolves multiply:i8_iday is one of them. --- .../type/TypeExpressionEvaluator.java | 5 +-- .../type/ParameterizedReturnTypeTest.java | 34 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java index 445fdd7a0..581ea10cc 100644 --- a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java +++ b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java @@ -27,8 +27,9 @@ * type classes whose parameter is an integer to substitute: {@code DECIMAL}, {@code * varchar}, {@code fixedchar}, {@code fixedbinary}, {@code precision_time

}, {@code * precision_timestamp

}, {@code precision_timestamp_tz

}, {@code interval_day

} and {@code - * interval_compound

}. The last two of those are supported for symmetry; no standard extension - * declares them parameterized today. + * interval_compound

}. No standard extension returns a parameterized {@code fixedbinary} or + * {@code interval_compound} -- those two are supported for symmetry, and pinned against + * hand-written declarations rather than the catalog. * *

What still fails on the standard extension catalog is a return of {@code list}, whose * parameter is a type to evaluate rather than an integer to substitute ({@code filter}, {@code diff --git a/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java index ad960d8db..3afa2b8ec 100644 --- a/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java +++ b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java @@ -85,6 +85,40 @@ void theOtherTemporalShapesCarryTheirPrecision() { resolve("add:ptstz_iyear_str", R.precisionTimestampTZ(9), R.INTERVAL_YEAR, R.STRING)); } + /** + * fixedbinary and interval_compound are the two parameterized shapes no standard extension + * declares as a return, so they are pinned against a hand-written declaration instead of the + * catalog. + */ + @Test + void theShapesTheCatalogDoesNotDeclareDeriveToo() { + assertEquals( + R.fixedBinary(9), + derive( + ParameterizedType.FixedBinary.builder().nullable(false).length(parameter("L1")).build(), + R.fixedBinary(9))); + assertEquals( + R.intervalCompound(3), + derive( + ParameterizedType.IntervalCompound.builder() + .nullable(false) + .precision(parameter("P")) + .build(), + R.intervalCompound(3))); + } + + private static ParameterizedType.StringLiteral parameter(String name) { + return ParameterizedType.StringLiteral.builder().nullable(false).value(name).build(); + } + + /** Derives the return of a one-argument declaration whose argument has the return's own shape. */ + private static Type derive(ParameterizedType declaredReturn, Type actual) { + return TypeExpressionEvaluator.evaluateExpression( + declaredReturn, + List.of(SimpleExtension.ValueArgument.builder().value(declaredReturn).name("arg1").build()), + List.of(actual)); + } + @Test void mirrorNullabilityStillApplies() { // The declared return is non-null; MIRROR makes it nullable because an argument is. From 55e0a3557f5bb31f0a7ef4672c8771aaea3d8f24 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Thu, 3 Sep 2026 20:37:00 +0300 Subject: [PATCH 4/5] fix(core): refuse an argument shape the declaration does not describe bind() walked past a declared/actual pair whose classes did not match, binding nothing and saying nothing, so the shared-parameter rule this PR extends was enforced for some calls and skipped for others: concat:vchar(varchar(20), string) derived varchar(20) where main threw, and isthmus reaches it because IgnoreNullableAndParameters.visit(Type.Str) accepts a string operand against a declared varchar. A terminal else fails closed, the way FunctionBindingResolver.typeMatches already does. List, map and struct declarations stay exempt. Binding never descends into them, so a mismatch there cannot be told from a shape this method does not reach yet, and refusing them would reject calls that resolve today: cardinality:list and index_in:any_list declare a concrete i64 return and need no binding at all. The inconsistent-binding test asserts the message it is named for now: contains("P") also passed on main, where the same call throws for an unsupported shape instead, so it could not tell the two apart. Both Javadocs listing what this validation still rejects lose their spec version, which lives in gradle/libs.versions.toml and has moved to 0.102.0 since. The one on AggregateConversion names the two standard aggregates that reach that mode, quantile and avg over a decimal, rather than scalar functions it never sees. --- .../type/TypeExpressionEvaluator.java | 26 +++++++++- .../type/ParameterizedReturnTypeTest.java | 47 +++++++++++++++++-- .../isthmus/AggregateConversion.java | 13 ++--- 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java index 581ea10cc..ad6d10b13 100644 --- a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java +++ b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java @@ -40,7 +40,7 @@ * assume_timezone:date_str_i8} and the {@code strptime_*} family. Among the standard aggregates, * {@code quantile} cannot be derived at all: its declared return {@code LIST?} uses a plain * {@code any}, which carries no identity to bind. Both lists are pinned against the catalog by - * {@code ParameterizedReturnTypeTest}, which is spec v0.101.0 today. + * {@code ParameterizedReturnTypeTest}. */ public class TypeExpressionEvaluator { @@ -229,9 +229,33 @@ private void bind(ParameterizedType declared, Type actual, boolean bindNames) { ((ParameterizedType.IntervalCompound) declared).precision().value(), ((Type.IntervalCompound) actual).precision(), bindNames); + } else if (!(declared instanceof Type) && !isContainer(declared)) { + // A shape one of the arms above should have taken: the declaration carries a parameter and + // the actual type is not the class that would bind it. Binding nothing here would enforce + // the shared-parameter rule for some calls and skip it for others. + throw new UnsupportedOperationException( + String.format( + "Cannot bind parameters from declared argument type %s to actual type %s", + declared, actual)); } } + /** + * Whether the declared type holds other types rather than an integer parameter. Binding does + * not descend into these, so their parameters bind nothing and a mismatch cannot be told from a + * shape this method simply does not reach yet -- unlike the classes above, refusing here would + * reject declarations that resolve today without binding anything, such as a {@code list} + * argument to a function returning a concrete type. + * + * @param declared the declared argument type + * @return {@code true} if the type is a list, map or struct declaration + */ + private boolean isContainer(ParameterizedType declared) { + return declared instanceof ParameterizedType.ListType + || declared instanceof ParameterizedType.Map + || declared instanceof ParameterizedType.Struct; + } + private void bindType(String name, Type actual) { // Nullability is not part of a wildcard's identity: any1 binds to i32 and i32? alike, and the // return expression's own nullability (or the MIRROR policy) decides the result's. diff --git a/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java index 3afa2b8ec..45846ec98 100644 --- a/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java +++ b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java @@ -53,8 +53,8 @@ void intervalDayCarriesItsPrecision() { @Test void dateMinusIntervalDayDerivesAPrecisionTimestamp() { - // The signature #1117 is about: the spec declares precision_timestamp

, where P is the - // interval's, and nothing but the interval carries it. + // subtract(date, interval_day

) declares precision_timestamp

, and nothing but the + // interval carries P. assertEquals(R.precisionTimestamp(6), resolve("subtract:date_iday", R.DATE, R.intervalDay(6))); assertEquals(R.precisionTimestamp(3), resolve("subtract:date_iday", R.DATE, R.intervalDay(3))); } @@ -68,7 +68,48 @@ void oneParameterSharedByTwoArgumentsHasToAgree() { assertThrows( UnsupportedOperationException.class, () -> resolve("add_intervals:iday_iday", R.intervalDay(3), R.intervalDay(6))); - assertTrue(e.getMessage().contains("P"), e.getMessage()); + assertTrue( + e.getMessage().contains("Inconsistent binding for type parameter 'P'"), e.getMessage()); + } + + @Test + void aParameterizedDeclarationRejectsAnotherActualShape() { + UnsupportedOperationException e = + assertThrows( + UnsupportedOperationException.class, + () -> resolve("concat:vchar", R.varChar(20), R.STRING)); + + assertTrue( + e.getMessage().contains("Cannot bind parameters from declared argument type"), + e.getMessage()); + } + + @Test + void aContainerDeclarationIsNotRefusedForABindingItNeverMakes() { + // Binding does not descend into a list declaration, so a `list` argument binds nothing. + // Both of these declare a concrete return and need no binding at all, so refusing the shape + // would reject calls that resolve today. + assertEquals(R.I64, resolve("cardinality:list", R.list(R.I64))); + assertEquals(N.I64, resolve("index_in:any_list", R.I64, R.list(R.I64))); + } + + @Test + void aConcreteReturnStillChecksSharedParameters() { + UnsupportedOperationException comparison = + assertThrows( + UnsupportedOperationException.class, + () -> resolve("lt:any_any", R.precisionTimestamp(3), R.precisionTimestamp(6))); + assertTrue( + comparison.getMessage().contains("Inconsistent binding for type parameter 'any1'"), + comparison.getMessage()); + + UnsupportedOperationException strpos = + assertThrows( + UnsupportedOperationException.class, + () -> resolve("strpos:vchar_vchar", R.varChar(20), R.varChar(3))); + assertTrue( + strpos.getMessage().contains("Inconsistent binding for type parameter 'L1'"), + strpos.getMessage()); } @Test diff --git a/isthmus/src/main/java/io/substrait/isthmus/AggregateConversion.java b/isthmus/src/main/java/io/substrait/isthmus/AggregateConversion.java index 5a76b0a39..6eca842c6 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/AggregateConversion.java +++ b/isthmus/src/main/java/io/substrait/isthmus/AggregateConversion.java @@ -32,14 +32,11 @@ public enum FunctionBindingValidation { * *

Type derivation is fail-closed: a function whose return expression the derivation does not * yet support is rejected rather than assumed valid, so this mode is not adoptable for plans - * that use such functions. On the standard extension catalog what remains unsupported is a - * return of {@code list} ({@code filter}, {@code sort}, {@code transform}, {@code - * string_split}, {@code regexp_string_split}, {@code regexp_match_substring_all}) and a - * multi-line return program ({@code add}, {@code subtract}, {@code multiply}, {@code divide}, - * {@code modulus}, {@code ceil}, {@code floor}, {@code round}, the {@code bitwise_*} family, - * {@code assume_timezone} and {@code strptime_*}). {@code quantile}'s output type cannot be - * derived at all: its declared return {@code LIST?} uses a plain {@code any}, which - * carries no identity to bind (spec v0.101.0). + * that use such functions. Among the standard aggregates that means {@code quantile}, whose + * declared return {@code LIST?} uses a plain {@code any} carrying no identity to bind, and + * {@code avg} over a decimal, whose intermediate {@code STRUCT,i64>} is a + * parameterized struct the derivation has no case for. Because an unspecified aggregate phase + * consumes that intermediate state, an ordinary decimal {@code avg} is rejected too. */ EXTENSION_DECLARATION } From 4f9d5a6983990329000a43d9fa6a04ff6230e211 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 4 Sep 2026 10:05:19 +0300 Subject: [PATCH 5/5] fix(core): exempt a function declaration from the shape refusal too isContainer named list, map and struct but not func, so the terminal else took all_match:list_func and any_match:list_func down with it: both declare a concrete boolean? return and resolved before. A func plainly holds other types rather than an integer parameter, which is the rationale the method already states. filter and transform still fail, on their list return rather than on a binding error that would survive the container work. The census sweep behind the PR body could not see any of this: it excluded from its denominator exactly the variants that take a function argument. Widened, it puts the catalog at 579 resolved of 602 before the terminal else, 577 after, and 579 again with func exempted. The evaluator's Javadoc no longer names variants. The catalog is owned upstream, so a list of names here goes stale on a substrait-packaging bump with nothing to catch it; ParameterizedReturnTypeTest pins the names against the shipped declarations, and now also pins that the two shapes actually throw. Its hand-written fixtures use ParameterizedTypeCreator rather than a local copy of its parameter helper. --- .../type/TypeExpressionEvaluator.java | 32 +++++++------ .../type/ParameterizedReturnTypeTest.java | 46 +++++++++---------- 2 files changed, 39 insertions(+), 39 deletions(-) diff --git a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java index ad6d10b13..45f7e5e98 100644 --- a/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java +++ b/core/src/main/java/io/substrait/type/TypeExpressionEvaluator.java @@ -27,20 +27,21 @@ * type classes whose parameter is an integer to substitute: {@code DECIMAL}, {@code * varchar}, {@code fixedchar}, {@code fixedbinary}, {@code precision_time

}, {@code * precision_timestamp

}, {@code precision_timestamp_tz

}, {@code interval_day

} and {@code - * interval_compound

}. No standard extension returns a parameterized {@code fixedbinary} or - * {@code interval_compound} -- those two are supported for symmetry, and pinned against - * hand-written declarations rather than the catalog. + * interval_compound

}. No standard extension declares a parameterized {@code fixedbinary} or + * {@code interval_compound} at all, as an argument or as a return -- those two are supported for + * symmetry, and pinned against hand-written declarations rather than the catalog. * - *

What still fails on the standard extension catalog is a return of {@code list}, whose - * parameter is a type to evaluate rather than an integer to substitute ({@code filter}, {@code - * sort}, {@code transform}, {@code string_split}, {@code regexp_string_split}, {@code - * regexp_match_substring_all}), and a multi-line return program -- the decimal variants of {@code - * add}, {@code subtract}, {@code multiply}, {@code divide}, {@code modulus}, {@code ceil}, {@code - * floor}, {@code round} and the {@code bitwise_*} family, together with {@code - * assume_timezone:date_str_i8} and the {@code strptime_*} family. Among the standard aggregates, - * {@code quantile} cannot be derived at all: its declared return {@code LIST?} uses a plain - * {@code any}, which carries no identity to bind. Both lists are pinned against the catalog by - * {@code ParameterizedReturnTypeTest}. + *

A {@code list} return still fails whatever its element, because the evaluator does not descend + * into a container -- so an element parameter it would otherwise substitute, as in {@code + * list>}, is out of reach just as an element type to evaluate is. A multi-line return + * program still fails because evaluating one needs integer arithmetic over the bound parameters + * rather than substitution. And a plain {@code any} cannot be derived at all: unlike {@code any1} + * it names nothing, so there is no identity to bind. + * + *

Which shipped variants those cover is pinned by {@code ParameterizedReturnTypeTest} against + * the declarations the catalog ships, and deliberately not repeated here -- the catalog is owned + * upstream, so a list of names in this Javadoc would go stale on a {@code substrait-packaging} bump + * with nothing to catch it. */ public class TypeExpressionEvaluator { @@ -248,12 +249,13 @@ private void bind(ParameterizedType declared, Type actual, boolean bindNames) { * argument to a function returning a concrete type. * * @param declared the declared argument type - * @return {@code true} if the type is a list, map or struct declaration + * @return {@code true} if the type is a list, map, struct or function declaration */ private boolean isContainer(ParameterizedType declared) { return declared instanceof ParameterizedType.ListType || declared instanceof ParameterizedType.Map - || declared instanceof ParameterizedType.Struct; + || declared instanceof ParameterizedType.Struct + || declared instanceof ParameterizedType.Func; } private void bindType(String name, Type actual) { diff --git a/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java index 45846ec98..259161089 100644 --- a/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java +++ b/core/src/test/java/io/substrait/type/ParameterizedReturnTypeTest.java @@ -7,6 +7,7 @@ import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; import io.substrait.function.ParameterizedType; +import io.substrait.function.ParameterizedTypeCreator; import io.substrait.function.TypeExpression; import java.util.List; import java.util.stream.Collectors; @@ -86,11 +87,13 @@ void aParameterizedDeclarationRejectsAnotherActualShape() { @Test void aContainerDeclarationIsNotRefusedForABindingItNeverMakes() { - // Binding does not descend into a list declaration, so a `list` argument binds nothing. - // Both of these declare a concrete return and need no binding at all, so refusing the shape - // would reject calls that resolve today. + // Binding descends into none of the container declarations, so a `list` or a + // `func boolean?>` argument binds nothing. All four of these declare a concrete return + // and need no binding at all, so refusing the shape would reject calls that resolve today. assertEquals(R.I64, resolve("cardinality:list", R.list(R.I64))); assertEquals(N.I64, resolve("index_in:any_list", R.I64, R.list(R.I64))); + assertEquals(N.BOOLEAN, resolve("all_match:list_func", R.list(R.I64), N.BOOLEAN)); + assertEquals(N.BOOLEAN, resolve("any_match:list_func", R.list(R.I64), N.BOOLEAN)); } @Test @@ -133,23 +136,9 @@ void theOtherTemporalShapesCarryTheirPrecision() { */ @Test void theShapesTheCatalogDoesNotDeclareDeriveToo() { - assertEquals( - R.fixedBinary(9), - derive( - ParameterizedType.FixedBinary.builder().nullable(false).length(parameter("L1")).build(), - R.fixedBinary(9))); - assertEquals( - R.intervalCompound(3), - derive( - ParameterizedType.IntervalCompound.builder() - .nullable(false) - .precision(parameter("P")) - .build(), - R.intervalCompound(3))); - } - - private static ParameterizedType.StringLiteral parameter(String name) { - return ParameterizedType.StringLiteral.builder().nullable(false).value(name).build(); + ParameterizedTypeCreator P = ParameterizedTypeCreator.REQUIRED; + assertEquals(R.fixedBinary(9), derive(P.fixedBinaryE("L1"), R.fixedBinary(9))); + assertEquals(R.intervalCompound(3), derive(P.intervalCompoundE("P"), R.intervalCompound(3))); } /** Derives the return of a one-argument declaration whose argument has the return's own shape. */ @@ -167,10 +156,10 @@ void mirrorNullabilityStillApplies() { } /** - * The two return shapes the evaluator does not derive, pinned by the variants that carry them so - * that the list in {@link TypeExpressionEvaluator}'s Javadoc cannot go stale on its own. A - * parameter that is a type to evaluate rather than an integer to substitute is the first; a - * multi-line return program is the second. + * The census of what the evaluator does not derive: a {@code list} return is the first shape, a + * multi-line return program the second. {@link TypeExpressionEvaluator}'s Javadoc describes those + * shapes and points here rather than naming variants, so this test is the only place a {@code + * substrait-packaging} bump can make the two disagree. */ @Test void theReturnShapesThatAreNotDerivedYet() { @@ -203,6 +192,15 @@ void theReturnShapesThatAreNotDerivedYet() { "strptime_timestamp:str_str_str_i8", "subtract:dec_dec"), variantsReturning(TypeExpression.ReturnProgram.class)); + + // The lists above pin which variants carry each shape; these pin that the shapes actually fail, + // so making one derivable cannot leave the census passing and the Javadoc stale. + assertThrows( + UnsupportedOperationException.class, + () -> resolve("string_split:vchar_vchar", R.varChar(20), R.varChar(20))); + assertThrows( + UnsupportedOperationException.class, + () -> resolve("add:dec_dec", R.decimal(10, 2), R.decimal(10, 2))); } private static List variantsReturning(Class returnShape) {