fix(starrocks): emit aggregation keys and sort tuples in materialized slot order - #1710
Draft
aocsa wants to merge 1 commit into
Draft
fix(starrocks): emit aggregation keys and sort tuples in materialized slot order#1710aocsa wants to merge 1 commit into
aocsa wants to merge 1 commit into
Conversation
… slot order The FE lists grouping expressions in GROUP BY order and the sort tuple's materialization exprs with the ordering keys first, but it serializes both output tuples' slots in ascending slot id, and every consumer above the node resolves the row through that descriptor order. Emitting in FE order shipped a permuted row whenever GROUP BY was not in ascending ref-id order (TPC-H q01, q03, q18). grouping_materialization_order pairs each key slot with its bare slot-ref grouping expression as a bijection and refuses anything else; sort_materialization_order applies the same rule to the sort tuple. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Draft
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
TPC-H q01 groups by (l_returnflag, l_linestatus) and orders by the same two keys. The FE lists the grouping expressions in GROUP BY order, but it materializes the aggregation output tuple's slots in ascending slot id (every aggregation-output slot is serialized with column_pos -1). Every consumer above the node resolves the row through that descriptor order. This fragment's output names, the sink row, the receiver's declared stream schema and the ordering slot refs all read the tuple the same way. The translator emitted the keys in GROUP BY order, so whenever GROUP BY was not in ascending ref-id order the engine row and the descriptor view were permutations of each other. The next hop then read a wrong column, with no error. q03 and q18 fail at the hop today with "stream 14 column 0 is declared DATE but the source sink produces BIGINT".
The sort tuple has the same problem. The FE builds it in two passes (ordering keys first, then the leftover payload slots in ascending slot id) and lists
sort_tuple_slot_exprsin that two-pass order, while the descriptor again holds the slots in ascending slot id. An ORDER BY on a non-leading slot shipped the key column first.Both nodes now emit their new tuple in the descriptor's materialized-slot order.
grouping_materialization_orderpairs each output key slot with the grouping expression that carries the same ref id (the FE reuses the expression's ref id as the output key slot id).sort_materialization_orderpairs each ordering expression with the sort-tuple slot it names and appends the remaining materialized slots as the payload.translate_aggregationiterateskey_orderfor the key-type check and the grouping expressions;translate_sorttranslates each materialization expression into a slot-keyed map and drains it in descriptor order.I made the pairing a strict bijection rather than a best effort. Every grouping expression must be a bare slot ref, no slot may appear twice, and every key slot must find its expression. Anything else means the FE laid the tuple out differently than this model, and reordering on a wrong model would ship wrong columns without an error, which is the exact failure this PR removes. A single key needs no pairing and skips the check, so a
GROUP BY CAST(x)with one key still translates. With several keys a non-slot-ref grouping expression is refused with an UnsupportedPlanNode error that says so. The sort side refuses ordering expressions that are not slot refs into the sort tuple for the same reason.How I tested it. On a GB200 box (aarch64) I ran the CI trio: cargo fmt, clippy with warnings as errors, and the CN test suite without the engine feature. All 207 tests pass, 7 of them new.
Tests: 7 new integration tests in
tests/translate.rs(135 to 142), all built around the q03 shape (keys 18, 13, 16 over a tuple serialized 13, 16, 18, 35).group_by_keys_out_of_slot_order_ship_tuple_slot_orderandorder_by_on_a_non_leading_sort_slot_ships_tuple_slot_ordertranslate both sides of a hop and assert the sender's produced (name, type) columns equal the receiver's declared stream columns.topn_over_group_by_keys_out_of_slot_order_resolves_both_sort_keyscomposes the two reorders under a merging exchange and checks both ordering keys resolve to the fields that hold them on each side. Two controls pin that already-ordered plans do not move. Two rejection tests cover the non-slot-ref and the unpaired-slot cases. Without the source change 5 of the 7 fail and the 2 controls pass. dev's three existing descriptor-side slot-order tests are unchanged; they assert the invariant this PR makes the emitted row obey.Q1's GROUP BY and ORDER BY translate correctly after this layer. Not handled here: avg (its two-column partial state and the finalizing division land in layer 4,
stacked/translator-avg-expansion), and the multi-key GROUP BY over a non-slot-ref expression, which is refused rather than guessed.Layer 3 of the translator stack; base
stacked/translator-two-phase-agg.Checklist
References
Refs #1110, which added the descriptor-side slot-order tests this PR makes the emitted row obey. Follows layer 2 (
stacked/translator-two-phase-agg), which refuses two-phase avg until layer 4.