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

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions