Skip to content

Refactor Isthmus function mapping into a unified bidirectional model #1012

Description

@nielspardon

Part of epic #1013 (canonical Substrait algebra + dialect-boundary unparse). This issue is
the function-mapping foundation: the unified model each mapping uses to carry a
canonical/synthetic binding plus an optional lossy Calcite binding.

Summary

The Isthmus Calcite ⇄ Substrait function-mapping code (scalar, aggregate, window) is
fragmented across three unrelated mechanisms and two asymmetric directions.
Knowledge about a single function correspondence is scattered across many files keyed
several different ways, the forward and reverse paths are not modelled as inverses, and the
generic matching engine hard-codes specific function names. This is a tracking issue for
replacing the current approach with a single, unified, bidirectional mapping model.

This is a refactor / architecture effort (behaviour-preserving through the first phases),
not a new feature. It also removes a long-standing correctness blocker (N:1 reverse mapping,
see #377).

Current state

Mechanism Responsibility
Static Sig(operator, name) tables (expression/FunctionMappings.java) ~90 hand-curated Calcite-operator → Substrait-name pairs
ScalarFunctionMapper implementations (expression/*Mapper.java) Per-function operand reshaping for awkward cases (TRIM, SQRT, POSITION, EXTRACT, PARSE_*)
Auto-generated operators (SimpleExtensionToSqlOperator + AutomaticDynamicFunctionMappingConverterProvider) Build Calcite operators + return-type inference from the Substrait YAML — but only for functions absent from the Sig tables
  • Forward (Calcite → Substrait) is a type-driven matcher (FunctionConverter.FunctionFinder.attemptMatch).
  • Reverse (Substrait → Calcite) is a separate name → operator multimap lookup
    (FunctionConverter.getSqlOperatorFromSubstraitFunc) with ad-hoc disambiguation.

Motivation — root causes

  1. No first-class "one function correspondence" object. A single function's mapping is
    spread across files keyed four ways (SqlOperator identity, lowercased name,
    SimpleExtension.Function.key(), SqlKind). std_dev/variance touches six files:
    AggregateFunctions, FunctionMappings, AggregateFunctionConverter.leadingEnumArgs,
    FunctionConverter.filterByDistribution, EnumConverter.calciteEnumMap, and
    PreCalciteAggregateValidator.
  2. The two directions are not provably inverse. Any N:1 forward mapping is a latent
    reverse-lookup landmine — getSqlOperatorFromSubstraitFunc throws
    IllegalStateException("Found N SqlOperators…") unless the collision happens to be one of
    exactly 5 operators in the hard-coded OPERATOR_RESOLVER, or the special-cased
    distribution enum. This is the blocker behind (isthmus) wrong Substrait to Calcite mapping for subtract:date_iday #377.
  3. Domain specifics leaked into the generic engine. FunctionConverter
    (distributionArgument/filterByDistribution, matchKeys req/opt) hard-codes specific
    function names. The engine should carry none.
  4. Two philosophies coexist unresolved. The Sig tables (reuse Calcite's built-in
    operators) vs. SimpleExtensionToSqlOperator (derive everything from YAML), split by
    getUnmappedFunctions instead of unified.
  5. Return-type disagreements tangled in. Calcite's inference differs from Substrait's
    nullability, forcing shadow SqlAggFunction subclasses in AggregateFunctions whose only
    job is to override inferReturnType — while SimpleExtensionToSqlOperator already does
    YAML-driven inference. (Related: [isthmus][SubstraitToCalcite] Aggregation nullability differs from Calcite #336.)

Debris: WindowRelFunctionConverter is dead code (never wired) duplicating
WindowFunctionConverter; two divergent CallConverter lists; a stale
"window functions not implemented" javadoc.

Proposed architecture

One bidirectional FunctionMapping per correspondence, owning both directions, its own
argument-shape adaptation, and its own reverse-disambiguation rule. A generic engine drives
matching and inference but carries no function-specific knowledge.

interface FunctionMapping<I /* Substrait invocation type */> {
  String substraitName();
  List<SqlOperator> calciteOperators();                 // size > 1 (N:1) is first-class
  Optional<List<RexNode>> adaptForward(GenericCall call, Context ctx);   // reorder/drop/inject; empty = decline
  List<RexNode> adaptReverse(I invocation, List<RexNode> genericArgs, Context ctx);
  default boolean claimsReverse(I invocation) { return true; }           // reverse disambiguation
}

Supporting pieces:

  • ArgAdapter — a small set of composable, inherently-invertible operand transforms
    (swap, dropLeading, injectConstant, enumFlag, foldVariadic). Written once; the
    reverse is derived. Subsumes every current reshaping hack (TRIM/POSITION/PARSE swaps,
    EXTRACT indexing injection, leadingEnumArgs, the CONCAT >2-arg reverse fold, and the
    matchKeys req/opt cross-product) — eliminating the duplicated Collections.swap pairs
    currently written twice per function.
  • Reverse via predicates (fixes N:1). Index mappings by Substrait name; on reverse,
    pick the candidate whose claimsReverse returns true. Overlapping predicates become a
    registration-time error naming the offending mappings, not a runtime exception on a
    random query. OPERATOR_RESOLVER's type-sets and filterByDistribution move into the
    individual mappings.
  • Per-mapping enum bindings replace the global EnumConverter.calciteEnumMap registry.
  • Unified return-type / nullability policy, YAML-driven, letting most AggregateFunctions
    shim subclasses be retired.
  • Unify mapped + dynamic paths — every Substrait function gets a mapping (bound to a
    standard Calcite operator when we want Calcite's parsing/optimizer/unparse, else to a
    synthetic operator). AutomaticDynamicFunctionMappingConverterProvider's behaviour becomes
    the default.

Scalar / aggregate / window stay separate. Substrait treats aggregate and window
functions as distinct categories (separate YAML sections, separate invocation types) —
only Calcite flattens them into "operators". The differences are essential, not incidental:
ScalarFunctionInvocation/WindowFunctionInvocation implement Expression while
AggregateFunctionInvocation does not; the field sets diverge (aggregate adds
options/aggregationPhase/sort/invocation; window additionally adds
partitionBy/boundsType/lowerBound/upperBound); and the Calcite inputs differ
(RexCall vs AggregateCall + RelNode input vs RexOver). So we keep three kind-specific
FunctionMapping<I> families and converters for the convert() edge and invocation
construction, and share only the kind-agnostic engine (matcher over GenericCall,
ArgAdapter, reverse-predicate resolution, enum bindings, return-type policy). This split
also keeps the door open to extracting a generic mapping framework into core (see Scope).

Worked example: std_dev / variance

Today: logic in six files. Proposed: one StatisticalFunctionMapping — operators
[STDDEV_POP, STDDEV_SAMP], forward enumFlag(0, StatisticalDistribution.class, …), reverse
strips the leading distribution EnumArg, claimsReverse matches the enum value. The engine,
EnumConverter, and PreCalciteAggregateValidator special-cases all disappear.

Scope

In scope: the mapping model for scalar, aggregate, and window functions in both
directions.

Out of scope (separate follow-up):

  • Multi-dialect SQL output (canonical Substrait-named operators + per-dialect
    SqlDialect.unparseCall). This work removes the N:1 blocker that currently stalls it, so
    it is best done afterward.
  • Unifying the parallel :spark Scala mapping implementation. Keeping the function kinds
    separate (above) plus the already-present but currently-unused AbstractFunctionInvocation<T, I>
    base in core is the natural seam for a future engine-agnostic mapping framework in core,
    shared by :isthmus and :spark. The FunctionMapping/ArgAdapter abstractions should
    therefore avoid gratuitous Calcite coupling so they can later be extracted. Out of scope for
    this effort, but explicitly designed for.

Public API impact

The mapping classes are effectively internal to :isthmus (:spark has its own independent
Scala implementation). The only external leakage is ConverterProvider and its subclasses
(used by isthmus-cli and examples/isthmus-api) and the FunctionMappings.Sig /
ScalarFunctionConverter(additionalSignatures) custom-function entry points.

Breaking changes are acceptable — we prefer the clean end state over source compatibility.
The provider hierarchy (DynamicConverterProvider / AutomaticDynamicFunctionMappingConverterProvider)
is collapsed into the unified model as a feat(isthmus)!: change, and the "register a Sig +
hand-built SqlFunction" custom-function story is replaced outright by "contribute a
FunctionMapping". No obligation to retain a source-compatible shim; isthmus-cli and the
examples/isthmus-api samples are updated in lockstep, and the breaking change is documented
with a migration note in the PR/changelog.

Phased plan (each phase independently shippable; existing round-trip suites are the oracle)

  • P0 — debris (no behaviour change): delete dead WindowRelFunctionConverter, merge
    the two CallConverter lists, fix the stale SimpleExtensionToSqlOperator javadoc.
  • P1 — introduce FunctionMapping + ArgAdapter: migrate the 7 ScalarFunctionMappers;
    remove the duplicated forward/reverse swap logic. Behaviour identical.
  • P2 — de-leak the engine: move std_dev/variance + enum-arg handling out of
    FunctionConverter/AggregateFunctionConverter/EnumConverter into mappings; delete
    distributionArgument, filterByDistribution, leadingEnumArgs, and the
    EnumConverter.calciteEnumMap distribution entries.
  • P3 — reverse via predicates: replace substraitFuncKeyToSqlOperatorMap +
    OPERATOR_RESOLVER + the IllegalStateException with per-mapping claimsReverse and a
    registration-time conflict check. Fixes (isthmus) wrong Substrait to Calcite mapping for subtract:date_iday #377 and unblocks new N:1 synonyms.
  • P4 — unify mapped/dynamic + return types: make "a mapping for every function" the
    default; consolidate return-type inference; retire redundant AggregateFunctions shims;
    collapse the provider hierarchy (breaking change, feat(isthmus)!:), updating
    isthmus-cli and the examples.

Testing strategy

Behaviour-preserving through P3; the existing round-trip suites are the oracle
(FunctionConversionTest, AggregationFunctionsTest, StatisticalFunctionTest,
StringFunctionTest, WindowFunctionTest, ArithmeticFunctionTest,
ComparisonFunctionsTest, RoundingFunctionTest, LogarithmicFunctionTest,
CustomFunctionTest, AutomaticDynamicFunctionMappingRoundtripTest, and the verifyRoundTrip
helpers). New tests worth adding: reverse-mapping a fresh N:1 synonym without an exception;
registration-time conflict detection firing for overlapping claimsReverse; and an
ArgAdapter invertibility property test (reverse(forward(x)) == x).

Open questions

(None blocking — all resolved below. Remaining implementation choices are cheaply empirical
and made during their phase, guarded by the round-trip suites.)

Resolved:

  • Return-type shims (MIN/MAX/SUM/AVG/SUM0) — decide empirically in P4; provisionally the
    unified policy replaces them.
    The five AggregateFunctions subclasses hand-encode what the
    spec YAML already declares: min/max/avg/sum are DECLARED_OUTPUT with a nullable return, which
    the YAML-driven policy renders nullable (matching ARG0_FORCE_NULLABLE); sum0 is
    DECLARED_OUTPUT i64 (non-null), matching ReturnTypes.BIGINT, and its name comes free from
    the YAML. So one generic return-type wrapper (standard Calcite operator + YAML-derived
    return type) should replace all five bespoke subclasses — keeping the standard operator
    identity for Calcite's optimizer/unparse while removing the hand-coding. Whether even the
    generic wrapper is needed (vs. going fully synthetic, or vs. stock Calcite inference) is
    verified per round-trip/nullability test in P4, not decided up front.
  • Source compatibility of the ConverterProvider surface is not a constraint — breaking
    changes are acceptable in favour of the clean end state.
  • Scalar / aggregate / window converters stay separate (see Proposed architecture) rather
    than collapsing into one — the invocation types and Calcite inputs differ essentially, and
    the separation supports a later engine-agnostic core framework.
  • Niladic context variables (CURRENT_TIMESTAMP/CURRENT_DATE/CURRENT_TIMEZONE) stay as
    dedicated CallConverters / expression visitors, not FunctionMappings. They are
    first-class Substrait ExecutionContextVariable expression types
    (Expression.CurrentTimestamp/CurrentDate/CurrentTimezone), not functions — only Calcite
    models them as niladic operators. Keeping them out of the function-mapping model keeps it
    functions-only and supports the core-extraction goal.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions