From edc848710041b969d922c7a1da183ebe7277a352 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ingo=20M=C3=BCller?= Date: Fri, 13 Sep 2024 08:44:22 +0000 Subject: [PATCH] [WIP] feat: change grouping expressions in AggregateRel to references. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 --- proto/substrait/algebra.proto | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/proto/substrait/algebra.proto b/proto/substrait/algebra.proto index eed7bcc79..847114d49 100644 --- a/proto/substrait/algebra.proto +++ b/proto/substrait/algebra.proto @@ -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 {