fix(core): preserve extended aggregate sorts and options

[bvolpato]

Extended-expression serialization drops aggregate sort fields and function options. An ordered string_agg therefore loses its ordering, and a SUM with overflow preferences loses the selected behavior. Functions referenced only by sort expressions also disappear from the extension declarations.

The loss occurs across the normal conversion path:

ExtendedExpression restored = new ProtoExtendedExpressionConverter().from(
    new ExtendedExpressionProtoConverter().toProto(expression));

Serialize the sort expressions and directions using the same expression converter and extension collector as the arguments, and preserve function-option preference order. This brings extended aggregates into line with aggregate-relation serialization.

Partially addresses #1093 for aggregate sorts and options.

[coderabbitai]

Warning

Review limit reached

Next included review available in 41 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: e1522170-5899-4d38-b540-07f10e921f62

📥 Commits

Reviewing files that changed from the base of the PR and between 4d21734 and 7238143.

📒 Files selected for processing (2)

• core/src/main/java/io/substrait/relation/AggregateFunctionProtoConverter.java
• core/src/test/java/io/substrait/extendedexpression/ExtendedExpressionRoundTripTest.java

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[nielspardon]

Thanks, the fix is correct and both new tests fail without it. Before merging, please make RelProtoConverter.toProto(Aggregate.Measure) delegate to AggregateFunctionProtoConverter instead of copying its lines, as #1093 proposes, so the next field added to AggregateFunction can't land in only one of the two writers again.

[nielspardon]

Add a constructor that takes the caller's ExpressionProtoConverter and ExtensionCollector, then have RelProtoConverter wrap the result with the filter (AggregateRel.Measure.newBuilder().setMeasure(aggFnConverter.toProto(measure))). This also lets ExtendedExpressionProtoConverter pass the expression converter it already has, instead of building a new converter per measure.

[nielspardon]

Once #1342 lands, delegate to its ExpressionProtoConverter.toProto(SortField). That PR makes direction() an Optional, so this line stops compiling, and a comparison-function sort has no direction to set anyway. Drop the io.substrait.proto.SortField import too.

Suggested change

	.addAllSorts(
	measure.getFunction().sort().stream()
	.map(
	sort ->
	SortField.newBuilder()
	.setExpr(exprProtoConverter.toProto(sort.expr()))
	.setDirection(sort.direction().toProto())
	.build())
	.collect(Collectors.toList()))
	.addAllSorts(
	measure.getFunction().sort().stream()
	.map(exprProtoConverter::toProto)
	.collect(Collectors.toList()))