Skip to content

Commit

Permalink
[WIP] feat: Deduplicate grouping expressions in AggregateRel.
Browse files Browse the repository at this point in the history
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 fc4a59e
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 fc4a59e

Please sign in to comment.