Skip to content

[isthmus][SubstraitToCalcite] Preserve declared aggregate output types #1016

Description

@rkondakov

Summary

SubstraitToCalcite does not reliably preserve the
AggregateFunctionInvocation.outputType carried by a Substrait plan. It initially puts the
declared type on the AggregateCall, but
RelBuilder.aggregate(groupKey, aggregateCalls) rebuilds the calls and lets Calcite infer their
return types again.

As a result, a self-describing Substrait plan can change aggregate width and/or nullability during
Substrait → Calcite conversion.

Reproduced on released io.substrait:isthmus:0.88.0 and on main at 48133f9.

Minimal reproduction

ConverterProvider provider = new ConverterProvider();
SubstraitBuilder b = new SubstraitBuilder(provider.getExtensions());
TypeCreator R = TypeCreator.of(false);
TypeCreator N = TypeCreator.of(true);

Rel input =
    b.namedScan(
        List.of("example"),
        List.of("i", "f", "group_key"),
        List.of(R.I32, R.FP32, N.STRING));

Rel aggregate =
    b.aggregate(
        rel -> b.grouping(rel, 2),
        rel -> List.of(
            withOutputType(b.sum(rel, 0), R.I64),
            withOutputType(b.avg(rel, 1), R.FP32)),
        input);

RelNode converted = new SubstraitToCalcite(provider).convert(aggregate);

Expected output types:

  • SUM: required i64 (BIGINT NOT NULL)
  • AVG: required fp32 (REAL NOT NULL)

Current behavior:

  • SUM becomes nullable INTEGER, re-derived from its i32 argument
  • AVG keeps the base REAL type but loses the declared required nullability

The first mismatch is observed as:

expected: BIGINT
 but was: INTEGER

Why this is distinct from #336

#336 discusses which nullability an aggregate should be declared with when Substrait and Calcite
inference differ. This issue is narrower: once a concrete outputType is already present in the
plan, the transport conversion should preserve it exactly rather than replace it with another
inference policy.

Consumers can then apply their own compatibility/subtyping policy. The converter should not
silently alter the declaration.

This is also related to the return-type work planned in #1012, but it is a standalone round-trip
correctness defect and can be fixed independently.

Matching compatibility is a separate consumer policy

This issue does not propose making the declared output type part of SqlOperator.equals.
Whether, for example, a wider or non-nullable aggregate can satisfy another plan is a consumer's
matching/subtyping policy rather than a conversion rule.

If the implementation carries the declaration in a private operator wrapper, consider exposing it
through a small public helper such as:

Optional<RelDataType> AggregateFunctions.declaredOutputType(SqlAggFunction function)

That would be symmetric with AggregateFunctions.withoutDeclaredOutputType(...) and would let
consumers implement an explicit policy without depending on a private wrapper. Consumers operating
on an AggregateCall can also read call.getType().

Acceptance criteria

  • Substrait → Calcite preserves the exact declared aggregate output type, including width,
    precision/scale, and nullability.
  • At minimum, regression coverage includes SUM width + nullability and AVG nullability.
  • Substrait → Calcite → Substrait preserves those declared types.
  • Calcite → Substrait aggregate function lookup still works for any per-invocation
    wrapper/operator used to carry the declared type.
  • Operator equality remains type-agnostic unless a consumer explicitly adds its own compatibility
    policy.

A working patch and regression test are ready locally.

Metadata

Metadata

Assignees

Labels

No labels
No 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