core: SortField cannot represent comparison_function_reference, so plans using it fail to read #1330

Description

SortField in algebra.proto makes its sort kind a oneof: either a direction or a comparison_function_reference that "references a function_anchor defined in this plan". Expression.SortField models only expr() and direction(), so the second form has no POJO representation.

message SortField {
  Expression expr = 1;

  oneof sort_kind {
    SortDirection direction = 2;
    // References a function_anchor defined in this plan.
    uint32 comparison_function_reference = 3;
  }
  ...
}

What core does today

• proto → POJO: ProtoExpressionConverter.fromSortField reads s.getDirection() unconditionally. For a sort using comparison_function_reference that returns SORT_DIRECTION_UNSPECIFIED, and SortDirection.fromProto throws IllegalArgumentException("Unknown type: SORT_DIRECTION_UNSPECIFIED"). So any plan with a custom-comparison sort, in any relation or function that carries sorts, fails to read.
• POJO → proto: RelProtoConverter.toProtoS and ExpressionProtoConverter always set direction, so a producer cannot emit one.

Suggested shape

• Model the oneof on Expression.SortField, e.g. an optional direction() plus an optional comparison-function reference, with a @Value.Check that exactly one is set. The reference should resolve to a function declaration the way other function references do, so the plan's extension collection carries it on the way out.
• Update both converters and every sorts-carrying site that constructs or copies a SortField (RelCopyOnWriteVisitor, ExpressionCopyOnWriteVisitor, ProtoRelConverter, ProtoAggregateFunctionConverter), plus a round-trip test.
• Making direction() optional is a source-breaking change for callers that read it.

Follow-on

Once this is modeled, the remaining half of the RANGE-frame sort rule from #1198 becomes enforceable: for RANGE with a Preceding or Following bound, the single ordering expression "must not use SORT_DIRECTION_CLUSTERED or a custom comparison function". #1205 already covers the CLUSTERED half, and the check can be extended there.

🤖 Generated with AI