The spec gives an aggregate's direct output order as "The list of grouping expressions in declaration order followed by the list of measures in declaration order, followed by an i32 describing the associated particular grouping set the value is derived from (if applicable)" (logical relations, spec v0.101.0). The POJO cannot hold AggregateRel.grouping_expressions directly — it models a per-Grouping expression list — so the shared list is reconstructed as the distinct grouping expressions in first-appearance order across the sets. That reconstruction is currently derived independently in six places, over two different equality relations:
| Site |
Spelling |
Equality |
core Aggregate.java:48 |
LinkedHashSet<Expression> |
Substrait Expression, includes the declared type |
core RelProtoConverter.java:240 |
.distinct() |
Substrait Expression |
isthmus SubstraitRelVisitor.java:401 |
.distinct().count() |
Substrait Expression |
isthmus SubstraitRelVisitor.java:518 |
.distinct() |
Substrait Expression |
isthmus SubstraitRelNodeConverter.java:370 |
LinkedHashSet<RexNode> |
Calcite RexNode |
isthmus SubstraitRelNodeConverter.java:447 |
LinkedHashSet<RexNode> |
Calcite RexNode |
The two isthmus RexNode sites are the ones that can disagree with the rest: RexInputRef.equals compares the index only and ignores the type, so two grouping expressions naming the same offset with different declared types are one column there and two in Aggregate.deriveRecordType(). Aggregate.deriveRecordType() also dedups only when getGroupings().size() > 1 while the proto writer at RelProtoConverter.java:240 dedups unconditionally, so core already disagrees with itself for a single grouping set.
The rule belongs on the POJO once — something like a static Aggregate.groupingColumns(List<Grouping>) plus an instance accessor — with deriveRecordType(), RelProtoConverter.visit(Aggregate) and both isthmus directions reading it from there. That would also make the grouping-set index's position a modeled fact rather than the arithmetic coincidence it is in isthmus today.
Any future clarification of how the spec treats structurally-equal-but-distinct grouping expressions currently has to be applied in six places, and nothing in the build fails when one is missed. Found while reviewing #1161.
The spec gives an aggregate's direct output order as "The list of grouping expressions in declaration order followed by the list of measures in declaration order, followed by an
i32describing the associated particular grouping set the value is derived from (if applicable)" (logical relations, spec v0.101.0). The POJO cannot holdAggregateRel.grouping_expressionsdirectly — it models a per-Groupingexpression list — so the shared list is reconstructed as the distinct grouping expressions in first-appearance order across the sets. That reconstruction is currently derived independently in six places, over two different equality relations:coreAggregate.java:48LinkedHashSet<Expression>Expression, includes the declared typecoreRelProtoConverter.java:240.distinct()ExpressionisthmusSubstraitRelVisitor.java:401.distinct().count()ExpressionisthmusSubstraitRelVisitor.java:518.distinct()ExpressionisthmusSubstraitRelNodeConverter.java:370LinkedHashSet<RexNode>RexNodeisthmusSubstraitRelNodeConverter.java:447LinkedHashSet<RexNode>RexNodeThe two isthmus
RexNodesites are the ones that can disagree with the rest:RexInputRef.equalscompares the index only and ignores the type, so two grouping expressions naming the same offset with different declared types are one column there and two inAggregate.deriveRecordType().Aggregate.deriveRecordType()also dedups only whengetGroupings().size() > 1while the proto writer atRelProtoConverter.java:240dedups unconditionally, so core already disagrees with itself for a single grouping set.The rule belongs on the POJO once — something like a static
Aggregate.groupingColumns(List<Grouping>)plus an instance accessor — withderiveRecordType(),RelProtoConverter.visit(Aggregate)and both isthmus directions reading it from there. That would also make the grouping-set index's position a modeled fact rather than the arithmetic coincidence it is in isthmus today.Any future clarification of how the spec treats structurally-equal-but-distinct grouping expressions currently has to be applied in six places, and nothing in the build fails when one is missed. Found while reviewing #1161.