Skip to content

Commit

Permalink
[WIP] feat: change grouping expressions in AggregateRel to references.
Browse files Browse the repository at this point in the history
BREAKING CHANGE: This PR changes the definition of grouping sets in
`AggregateRel` to consist of references into a list of grouping
expressions instead of consisting of expressions directly. As discussed
in more detail in substrait-io#700, the previous version is problematic because it
requires consumers to deduplicate these expressions, which, in turn,
requires to parse and understand 100% of these expression even in cases
where that understanding is otherwise optional. The new version avoids
that problem and, thus, allows consumers to be simpler.

Signed-off-by: Ingo Müller <ingomueller@google.com>
  • Loading branch information
ingomueller-net committed Sep 13, 2024
1 parent 16f7083 commit 64e9e71
Showing 1 changed file with 15 additions and 3 deletions.
18 changes: 15 additions & 3 deletions proto/substrait/algebra.proto
Original file line number Diff line number Diff line change
Expand Up @@ -244,18 +244,30 @@ message AggregateRel {
// Input of the aggregation
Rel input = 2;

// A list of one or more grouping expression sets that the aggregation measures should be calculated for.
// Required if there are no measures.
// A list of zero or more grouping sets that the aggregation measures should
// be calculated for. There must be at least one grouping set if there are no
// measures (but it can be the empty grouping set).
repeated Grouping groupings = 3;

// A list of one or more aggregate expressions along with an optional filter.
// Required if there are no groupings.
repeated Measure measures = 4;

// A list of zero or more grouping expressions that grouping sets (i.e.,
// `Grouping` messages in the `groupings` field) can reference. Each
// expression in this list must be referred to by at least one
// `Grouping.expression_reference`.
repeated Expression grouping_expressions = 5;

substrait.extensions.AdvancedExtension advanced_extension = 10;

message Grouping {
repeated Expression grouping_expressions = 1;
// Deprecated in favor of `expression_reference` below.
repeated Expression grouping_expressions = 1 [deprecated = true];

// A list of zero or more references to grouping expressions, i.e., indices
// into the `grouping_expression` list.
repeated uint32 expression_reference = 2;
}

message Measure {
Expand Down

0 comments on commit 64e9e71

Please sign in to comment.