GROUP_ID remap resolves measures with AggregateCall.equals, which ignores the declared type

[nielspardon]

Summary

The GROUP_ID() remap in visit(Aggregate) resolves each measure to its output slot with List.indexOf, which uses AggregateCall.equals. That comparison does not include the call's declared output type, so two measures differing only in their type both resolve to the first one's index — dropping one measure's column and giving it the other's type. Found while reviewing #1090.

Where

isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java:434-448

for (int i = 0; i < aggregate.getAggCallList().size(); i++) {
  AggregateCall aggCall = aggregate.getAggCallList().get(i);
  if (filteredAggCalls.contains(aggCall)) {
    remap.add(
        i + groupingFieldCountWithDuplicates,
        filteredAggCalls.indexOf(aggCall) + groupingFieldCountWithDuplicates);
  } else if (groupIdCalls.contains(aggCall)) {
    ...

Both contains and indexOf go through AggregateCall.equals, which compares the function, distinct/approximate/ignoreNulls flags, argument list, filter argument, distinct keys and collation — but not the declared type and not the name.

Why this is not hypothetical

The repo already knows AggregateCall.equals ignores the type, and works around it on the other side of the conversion. SubstraitRelNodeConverter.hasTypeDistinctDuplicates (isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java:438-447) exists solely to detect this case:

Map<AggregateCall, RelDataType> typesByCall = new HashMap<>();
for (AggregateCall call : aggregateCalls) {
  RelDataType existing = typesByCall.putIfAbsent(call, call.getType());
  if (existing != null && !existing.equals(call.getType())) {
    return true;
  }
}

and when it fires, :393-394 deliberately turns Calcite's own de-duplication off:

hasTypeDistinctDuplicates(aggregateCalls)
    ? relBuilder.transform(config -> config.withDedupAggregateCalls(false))

So the Substrait → Calcite direction goes out of its way to preserve two same-signature, different-type measures as two distinct calls, and the Calcite → Substrait direction then collapses them via indexOf.

Failure scenario

An aggregate that has grouping sets, a GROUP_ID() call (so the remap branch is taken at all), and two measures identical apart from their declared output type: the remap ends up with both output slots pointing at the first measure. One measure's column is silently unreachable and the other's type is applied to both. No exception.

Fix

Resolve by index rather than by value. filteredAggCalls is built by filtering aggregate.getAggCallList(), so the position is already known at the point the list is built — carrying it (or building an identity-keyed map) removes the contains / indexOf pair entirely and makes the remap independent of what AggregateCall.equals happens to compare.

Why the tests do not catch it

The remap branch requires grouping sets plus GROUP_ID(), and the collision additionally requires two type-distinct same-signature measures. No test combines the two, even though each half is covered separately.