From a54313f865efda5b0afb830fe5c2cdc17e7372ca Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Wed, 15 Jul 2026 16:12:55 +0200 Subject: [PATCH] refactor(isthmus): remove dead WindowRelFunctionConverter and de-duplicate call converters WindowRelFunctionConverter was never instantiated or wired into any ConverterProvider and duplicated WindowFunctionConverter; remove it. The WindowRelFunctionInvocation capability it targeted is produced only in core and the Spark integration, both untouched. Have ConverterProvider.getCallConverters() build on CallConverters.defaults() instead of re-listing the identical converters. The order, instances, and mutability of the returned list are unchanged, so this is behavior-preserving. Update SimpleExtensionToSqlOperator javadoc to reflect that window functions are supported (the class already handles them). No behavior change; verified with the isthmus test suite, PMD, spotless, and javadoc. First, low-risk step of the function-mapping refactor. Part of #1012 --- .../substrait/isthmus/ConverterProvider.java | 14 +- .../isthmus/SimpleExtensionToSqlOperator.java | 18 +- .../WindowRelFunctionConverter.java | 217 ------------------ 3 files changed, 11 insertions(+), 238 deletions(-) delete mode 100644 isthmus/src/main/java/io/substrait/isthmus/expression/WindowRelFunctionConverter.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index 8cd3731b3..12c8790a2 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -6,11 +6,8 @@ import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.CallConverters; import io.substrait.isthmus.expression.ExpressionRexConverter; -import io.substrait.isthmus.expression.FieldSelectionConverter; import io.substrait.isthmus.expression.RexExpressionConverter; import io.substrait.isthmus.expression.ScalarFunctionConverter; -import io.substrait.isthmus.expression.SqlArrayValueConstructorCallConverter; -import io.substrait.isthmus.expression.SqlMapValueConstructorCallConverter; import io.substrait.isthmus.expression.WindowFunctionConverter; import io.substrait.plan.ImmutableExecutionBehavior; import io.substrait.plan.Plan; @@ -266,15 +263,8 @@ public RexExpressionConverter getRexExpressionConverter(SubstraitRelVisitor srv) * @return a list of CallConverter instances */ public List getCallConverters() { - ArrayList callConverters = new ArrayList<>(); - callConverters.add(new FieldSelectionConverter(typeConverter)); - callConverters.add(CallConverters.CASE); - callConverters.add(CallConverters.ROW); - callConverters.add(CallConverters.CAST.apply(typeConverter)); - callConverters.add(CallConverters.REINTERPRET.apply(typeConverter)); - callConverters.add(CallConverters.EXECUTION_CONTEXT_VARIABLE); - callConverters.add(new SqlArrayValueConstructorCallConverter(typeConverter)); - callConverters.add(new SqlMapValueConstructorCallConverter()); + ArrayList callConverters = + new ArrayList<>(CallConverters.defaults(typeConverter)); callConverters.add(CallConverters.CREATE_SEARCH_CONV.apply(new RexBuilder(typeFactory))); callConverters.add(scalarFunctionConverter); return callConverters; diff --git a/isthmus/src/main/java/io/substrait/isthmus/SimpleExtensionToSqlOperator.java b/isthmus/src/main/java/io/substrait/isthmus/SimpleExtensionToSqlOperator.java index 1f56f0d52..652c04a34 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SimpleExtensionToSqlOperator.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SimpleExtensionToSqlOperator.java @@ -27,8 +27,8 @@ import org.slf4j.LoggerFactory; /** - * Utility class for converting Substrait {@link SimpleExtension} function definitions (scalar and - * aggregate) into Calcite {@link SqlOperator}s. + * Utility class for converting Substrait {@link SimpleExtension} function definitions (scalar, + * aggregate, and window) into Calcite {@link SqlOperator}s. * *

This enables Calcite to recognize and use Substrait-defined functions during query planning * and execution. Conversion includes: @@ -39,7 +39,7 @@ *

  • Inferring return types based on Substrait type expressions and nullability rules * * - *

    Currently supports scalar and aggregate functions; window functions are not yet implemented. + *

    Supports scalar, aggregate, and window functions. */ public final class SimpleExtensionToSqlOperator { @@ -53,8 +53,8 @@ public final class SimpleExtensionToSqlOperator { private SimpleExtensionToSqlOperator() {} /** - * Converts all functions in a Substrait {@link SimpleExtension.ExtensionCollection} (scalar and - * aggregate) into Calcite {@link SqlOperator}s using the default type factory. + * Converts all functions in a Substrait {@link SimpleExtension.ExtensionCollection} (scalar, + * aggregate, and window) into Calcite {@link SqlOperator}s using the default type factory. * * @param collection The Substrait extension collection containing function definitions. * @return A list of Calcite {@link SqlOperator}s corresponding to the Substrait functions. @@ -64,8 +64,8 @@ public static List from(SimpleExtension.ExtensionCollection collect } /** - * Converts all functions in a Substrait {@link SimpleExtension.ExtensionCollection} (scalar and - * aggregate) into Calcite {@link SqlOperator}s using a provided type factory. + * Converts all functions in a Substrait {@link SimpleExtension.ExtensionCollection} (scalar, + * aggregate, and window) into Calcite {@link SqlOperator}s using a provided type factory. * * @param collection The Substrait extension collection containing function definitions. * @param typeFactory Calcite {@link RelDataTypeFactory} for type creation and inference. @@ -77,8 +77,8 @@ public static List from( } /** - * Converts all functions in a Substrait {@link SimpleExtension.ExtensionCollection} (scalar and - * aggregate) into Calcite {@link SqlOperator}s with a custom type factory and {@link + * Converts all functions in a Substrait {@link SimpleExtension.ExtensionCollection} (scalar, + * aggregate, and window) into Calcite {@link SqlOperator}s with a custom type factory and {@link * TypeConverter}. * * @param collection The Substrait extension collection containing function definitions. diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/WindowRelFunctionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/WindowRelFunctionConverter.java deleted file mode 100644 index e75cc3ddd..000000000 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/WindowRelFunctionConverter.java +++ /dev/null @@ -1,217 +0,0 @@ -package io.substrait.isthmus.expression; - -import static io.substrait.isthmus.expression.WindowBoundConverter.toWindowBound; - -import com.google.common.collect.ImmutableList; -import io.substrait.expression.Expression; -import io.substrait.expression.ExpressionCreator; -import io.substrait.expression.FunctionArg; -import io.substrait.expression.WindowBound; -import io.substrait.extension.SimpleExtension; -import io.substrait.isthmus.AggregateFunctions; -import io.substrait.isthmus.TypeConverter; -import io.substrait.relation.ConsistentPartitionWindow; -import io.substrait.type.Type; -import java.util.List; -import java.util.Optional; -import java.util.function.Function; -import java.util.stream.Stream; -import org.apache.calcite.rel.core.Window; -import org.apache.calcite.rel.type.RelDataType; -import org.apache.calcite.rel.type.RelDataTypeFactory; -import org.apache.calcite.rex.RexNode; -import org.apache.calcite.rex.RexWindowBound; -import org.apache.calcite.sql.SqlAggFunction; - -/** - * Converts Calcite window aggregate calls (from {@link Window.RexWinAggCall}) into Substrait {@link - * ConsistentPartitionWindow.WindowRelFunctionInvocation}s. - * - *

    Handles bounds type (ROWS/RANGE), lower/upper bounds, DISTINCT/ALL invocation, and function - * signature matching against configured Substrait window function variants. - */ -public class WindowRelFunctionConverter - extends FunctionConverter< - SimpleExtension.WindowFunctionVariant, - ConsistentPartitionWindow.WindowRelFunctionInvocation, - WindowRelFunctionConverter.WrappedWindowRelCall> { - - /** - * Returns the supported window function signatures used for matching. - * - * @return immutable list of supported signatures. - */ - @Override - protected ImmutableList getSigs() { - return FunctionMappings.WINDOW_SIGS; - } - - /** - * Creates a converter with the provided window function variants. - * - * @param functions Supported Substrait window function variants. - * @param typeFactory Calcite type factory for type handling. - */ - public WindowRelFunctionConverter( - List functions, RelDataTypeFactory typeFactory) { - super(functions, typeFactory); - } - - /** - * Creates a converter with provided function variants and additional signatures. - * - * @param functions Supported Substrait window function variants. - * @param additionalSignatures Extra signatures to consider during matching. - * @param typeFactory Calcite type factory for type handling. - * @param typeConverter Converter for Calcite/Substrait types. - */ - public WindowRelFunctionConverter( - List functions, - List additionalSignatures, - RelDataTypeFactory typeFactory, - TypeConverter typeConverter) { - super(functions, additionalSignatures, typeFactory, typeConverter); - } - - /** - * Generates a bound Substrait window relation function invocation for a matched call. - * - * @param call Wrapped window rel call, including bounds and the original win-agg call. - * @param function Selected Substrait function variant. - * @param arguments Converted Substrait function arguments. - * @param outputType Result type for the invocation. - * @return Built {@link ConsistentPartitionWindow.WindowRelFunctionInvocation}. - */ - @Override - protected ConsistentPartitionWindow.WindowRelFunctionInvocation generateBinding( - WrappedWindowRelCall call, - SimpleExtension.WindowFunctionVariant function, - List arguments, - Type outputType) { - Window.RexWinAggCall over = call.getWinAggCall(); - - Expression.AggregationInvocation invocation = - over.distinct - ? Expression.AggregationInvocation.DISTINCT - : Expression.AggregationInvocation.ALL; - - // Calcite only supports ROW or RANGE mode - Expression.WindowBoundsType boundsType = - call.isRows() ? Expression.WindowBoundsType.ROWS : Expression.WindowBoundsType.RANGE; - WindowBound lowerBound = toWindowBound(call.getLowerBound()); - WindowBound upperBound = toWindowBound(call.getUpperBound()); - - return ExpressionCreator.windowRelFunction( - function, - outputType, - Expression.AggregationPhase.INITIAL_TO_RESULT, - invocation, - boundsType, - lowerBound, - upperBound, - arguments); - } - - /** - * Attempts to convert a Calcite {@link Window.RexWinAggCall} into a Substrait window relation - * function invocation. - * - *

    Resolves the corresponding Substrait aggregate function variant, checks arity using - * signatures, and builds the invocation if match succeeds. - * - * @param winAggCall Calcite window aggregate call. - * @param lowerBound Lower bound of the window. - * @param upperBound Upper bound of the window. - * @param isRows Whether the window uses ROWS (true) or RANGE (false). - * @param topLevelConverter Function converting top-level {@link RexNode}s to Substrait {@link - * Expression}s. - * @return {@link Optional} containing the {@link - * ConsistentPartitionWindow.WindowRelFunctionInvocation} if matched; otherwise empty. - */ - public Optional convert( - Window.RexWinAggCall winAggCall, - RexWindowBound lowerBound, - RexWindowBound upperBound, - boolean isRows, - Function topLevelConverter) { - SqlAggFunction aggFunction = (SqlAggFunction) winAggCall.getOperator(); - - SqlAggFunction lookupFunction = - AggregateFunctions.toSubstraitAggVariant(aggFunction).orElse(aggFunction); - FunctionFinder m = signatures.get(lookupFunction); - if (m == null) { - return Optional.empty(); - } - if (!m.allowedArgCount(winAggCall.getOperands().size())) { - return Optional.empty(); - } - - WrappedWindowRelCall wrapped = - new WrappedWindowRelCall(winAggCall, lowerBound, upperBound, isRows); - return m.attemptMatch(wrapped, topLevelConverter); - } - - static class WrappedWindowRelCall implements FunctionConverter.GenericCall { - private final Window.RexWinAggCall winAggCall; - private final RexWindowBound lowerBound; - private final RexWindowBound upperBound; - private final boolean isRows; - - private WrappedWindowRelCall( - Window.RexWinAggCall winAggCall, - RexWindowBound lowerBound, - RexWindowBound upperBound, - boolean isRows) { - this.winAggCall = winAggCall; - this.lowerBound = lowerBound; - this.upperBound = upperBound; - this.isRows = isRows; - } - - @Override - public Stream getOperands() { - return winAggCall.getOperands().stream(); - } - - @Override - public RelDataType getType() { - return winAggCall.getType(); - } - - /** - * Returns the underlying Calcite window aggregate call. - * - * @return the {@link Window.RexWinAggCall}. - */ - public Window.RexWinAggCall getWinAggCall() { - return winAggCall; - } - - /** - * Returns the lower bound of the window. - * - * @return the {@link RexWindowBound} lower bound. - */ - public RexWindowBound getLowerBound() { - return lowerBound; - } - - /** - * Returns the upper bound of the window. - * - * @return the {@link RexWindowBound} upper bound. - */ - public RexWindowBound getUpperBound() { - return upperBound; - } - - /** - * Whether the window uses ROWS (true) or RANGE (false). - * - * @return {@code true} if ROWS; {@code false} if RANGE. - */ - public boolean isRows() { - return isRows; - } - } -}