fix(isthmus): preserve field types in sort and aggregate references - #1090
Conversation
Use the referenced input field type for aggregate sort expressions and cover direct conversion plus full round-trip behavior.
nielspardon
left a comment
There was a problem hiding this comment.
The type fix is the right one, and it's worth stating that it repairs more than the title says: toSortField is shared with visit(Sort), so this also makes every Calcite-derived Sort rel round-trip-stable. I checked by reverting only the production line — assertFullRoundTrip("SELECT L_ORDERKEY FROM LINEITEM ORDER BY L_ORDERKEY") fails on main and passes with your change.
Three asks that have no line in the diff to hang off:
- The
fromAggCallJavadoc (lines 509-514) attributes the fp64 rewrite to this method and describes it as casting the argument in place. It's actuallycastStatisticalAggregatesToFloatingPoint, called fromvisit(Aggregate), and it appends fp64 columns and re-points the call. This diff removes the method's only other comment, so that paragraph is now its whole description. - Please add one
assertFullRoundTripwith anORDER BYto pin theSort-rel half. Nothing currently covers it:ComplexSortTestcompares Calcite explain text,SubstraitRelNodeConverterTest$Sortasserts onlygetRowType(), andsimpleOrderByClauseusesassertSqlSubstraitRelRoundTrip, which compares two POJOs both produced bySubstraitRelVisitorand so is blind to a uniformly-wrong type. - Since the squash message becomes the changelog entry, could the title name the real defect and the
Sortscope rather than "preserve aggregate collations"? Collations have in fact been propagated since the initial commit (AggregateFunctionConverter.sortFields), which your body says correctly — the removed TODO was just stale, so #215's literal text was already satisfied and the type defect is the fix worth advertising. I'd also drop the "cover mixed direction and null-order collations…" sentence; the diff shows the tests.
Everything else I ran into is pre-existing and out of scope here, and has been filed as issues.
|
Addressed the requested changes in 56106e7: fixed the pre-measure filter field type, switched sort references to constructOnRoot, strengthened the null-order expectations, corrected the fromAggCall Javadoc, and added a full ORDER BY round-trip. I also updated the title and description to describe the field-type defect and Sort scope. I left NullDirection.UNSPECIFIED and the optional test move out of this PR as suggested. |
nielspardon
left a comment
There was a problem hiding this comment.
All addressed — thanks. The UNSPECIFIED null-direction case you left out is tracked in #1092.
Calcite collations were already propagated to Substrait sort fields, but those field references used the whole input struct as their type. The shared conversion is used by both aggregate sort arguments and Sort relations, so those plans were not round-trip stable. Aggregate pre-measure filters had the same problem.
Build each reference from the selected input field, preserving its exact type in Sort relations, aggregate sort arguments, and pre-measure filters.
Closes #215