fix: give a multi-set aggregate the grouping set index Substrait defines - #25527
Open
namanjain24-sudo wants to merge 1 commit into
Open
namanjain24-sudo wants to merge 1 commit into
namanjain24-sudo wants to merge 1 commit into
Conversation
Substrait ends an AggregateRel with more than one grouping set with an i32 holding the zero-based index of the set that produced the row. DataFusion ends the same aggregate with __grouping_id, which packs a bitmask of the columns a set leaves out with an ordinal that separates repeated sets. The consumer mapped one onto the other, so a consumed plan returned the bitmask where the spec asks for the index, and the producer wrote the bitmask into the column another engine reads as the index. Both identify the set a row came from, so each is now written as a map of the other: the consumer projects the index, and the producer projects __grouping_id back from the index above an AggregateRel that carries what the spec defines.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25527 +/- ##
==========================================
- Coverage 82.38% 82.38% -0.01%
==========================================
Files 1138 1139 +1
Lines 434328 434457 +129
Branches 434328 434457 +129
==========================================
+ Hits 357824 357918 +94
- Misses 54872 54885 +13
- Partials 21632 21654 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Which issue does this PR close?
__grouping_id, not the set's index #25208.Rationale for this change
Substrait ends an
AggregateRelwith more than one grouping set with an extrai32whose value is "the zero-based index of the grouping set that yielded the record" (Aggregate Operation).DataFusion ends the same aggregate with
__grouping_id, which packs two things: a bitmask with a bit set for every grouping column the set leaves out, counting from the last column, and an ordinal that separates repeated sets. The consumer and the producer both treated that column as Substrait's index, so:UInt8rather thanInt32;The two coincide for some lists of sets, which is why this went unnoticed: for
(a, b)then(a)both are 0 then 1. For(a)then(b)the bitmask is 1 then 2 while the index is 0 then 1.What changes are included in this PR?
Both values identify the set a row came from, and every set has its own
__grouping_id, so each side can be written as a map of the other. The maps live in a newlogical_plan::grouping_setmodule shared by the two directions:CASE WHEN __grouping_id = <id of set 0> THEN 0 ... ELSE <last index> END, so the plan ends with the index, as a requiredInt32.AggregateRelholding what the spec defines and projects__grouping_idback from the index above it, which is also where the reordering to DataFusion's[groups, grouping_id, measures]now happens. A plan DataFusion writes therefore reads correctly in another engine, and still round trips here.The last set is the
ELSEarm rather than aWHEN: the values are exhaustive, and anELSEkeeps the column non-nullable, which Substrait requires of the index and DataFusion of__grouping_id.Two details the maps have to respect:
ROLLUPorCUBE, so the producer writes both as a list of sets, reversed forROLLUPand as a powerset forCUBE. The index follows that list, so the expansion is now one function used both to write the groupings and to compute the ids.GROUPING SETS ((a), (a))is two sets with two indexes, which DataFusion separates with the ordinal packed above the bitmask, so the map stays one-to-one.A single grouping set has no index column and is untouched, as is
SELECT DISTINCT.What is the testing strategy for this PR?
Consumer, in
aggregation_tests.rs:multiple_grouping_sets_emit_the_set_indexis the issue's plan: sets(a)then(b), where the index differs from the bitmask. It checks the column is a requiredInt32holding 0 and 1, wheremaingives aUInt8holding 1 and 2.duplicate_grouping_sets_are_separate_indexes: the same set twice gets index 0 and 1, with its rows once per index.Round trip, in
roundtrip_logical_plan.rs:aggregate_grouping_sets_keep_grouping_function:GROUPING(a)reads__grouping_id, so this only holds if the index is mapped back to it. The sets are(a),(c),(a, c), chosen so the index and the bitmask differ; with(a, c)first the test would pass either way.aggregate_duplicate_grouping_sets:GROUPING SETS ((a), (a), ())keeps each occurrence's rows.aggregate_grouping_sets_wider_grouping_id: nine grouping columns, so__grouping_idis aUInt16and the map has to carry the wider literal.aggregate_grouping_setsnow asserts that theAggregateRelno longer remaps its output and that the projection above it carries the map and the mapping.Each half was reverted on its own to check the tests pin it:
multiple_grouping_sets_emit_the_set_index,aggregate_duplicate_grouping_setsandaggregate_grouping_sets_keep_grouping_functionfail;aggregate_grouping_sets;UInt16arm of the literal narrowed toUInt8, onlyaggregate_grouping_sets_wider_grouping_idfails.cargo test -p datafusion-substraitpasses (58 unit, 215 integration with the 6 already ignored, 3 doc tests), as docargo fmt --all -- --check,cargo clippy -p datafusion-substrait --all-targets --features physical -- -D warningsandcargo xtask ci step test substrait. A Substrait round trip ofaggregate.slt, which is not in that job, reports the same 10 pre-existing failures asmain.Are there any user-facing changes?
Plans consumed from Substrait end a multi-set aggregate with the grouping set index, a required
Int32, instead of DataFusion's__grouping_id. Plans produced for a multi-set aggregate now carry a projection above theAggregateRelthat maps that index back to__grouping_id, so theAggregateRelitself holds the column the spec describes. No Rust API changes.