Skip to content

feat: add AggregateRel compatibility for grouping set #890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yongchul wants to merge 5 commits into
base: main
Choose a base branch
from
+43 −0
Open

feat: add AggregateRel compatibility for grouping set #890

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e0104c5
feat: aggregate rel compatibility options
yongchul Oct 13, 2025
4a2a735
feat: add AggregateRel compatibility
yongchul Nov 17, 2025
2949a95
Merge branch 'main' into grouping_set_behavior
yongchul Nov 17, 2025
f8f32d7
Adding compatibility documentation. Simplify field name
yongchul Dec 9, 2025
028b6d3
Change bool to enum. Updated documentation accordingly.
yongchul Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions proto/substrait/algebra.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,8 @@ message AggregateRel {
// `Grouping.expression_references`.
repeated Expression grouping_expressions = 5;

Compatibility compatibility = 6;
Comment thread
vbarua marked this conversation as resolved.

substrait.extensions.AdvancedExtension advanced_extension = 10;

message Grouping {
Expand All @@ -370,6 +372,14 @@ message AggregateRel {
// Helps to support SUM(<c>) FILTER(WHERE...) syntax without masking opportunities for optimization
Expression filter = 2;
}

// Various modes of operations of AggregateRel to capture different behaviors across systems.
message Compatibility {

@EpsilonPrime EpsilonPrime Dec 22, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit leery of having many submessages with similar names as that will complicate parsing. But having one compatibility message with many unused parts is similarly unsatisfying. The options behavior of functions is probably the most appropriate.

// If true, the AggregateRel must not yield a row on empty input
// when specified with non-empty groupings field even when groupings includes
// empty grouping sets.
bool groupings_yield_no_rows_on_empty_input = 1;
}

@vbarua vbarua Nov 20, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm off two minds on declaring settings like this. On one hand, having single message with a bunch of boolean behavioral toggles makes it easy to add new toggles, because we can just add a new field. We would need to make sure that the default unset value matched the default behaviour when we do this. Generally though, I'm wary of boolean toggles because IMO they can be hard to understand, and are limited to switching between 2 different behaviors.

I personally lean towards the enum style of setting toggles because we can indicate the expected behavior with the name, we can declare more than 2 types of behaviors, we can add behaviors easily if we discover more weird system behaviour and we can explicitly declare the unset values as unspecified.

message EmptyInputMode {
      OUTPUT_MODE_UNSPECIFIED = 0;
      OUTPUT_MODE_YIELD_EMPTY_ROW = 1;
      OUTPUT_MODE_YIELD_NO_ROW = 1;
}

@vbarua vbarua Dec 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we add these kinds of compatibility toggles, we should also document them in the website. It would be good to include the systems this is useful for, as well as example queries to trigger the behaviour in the docs for context as well.

@yongchul yongchul Dec 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I plan to add the documentation -- the weeks have been crazy, can't find time... perhaps later this week or next week, I'll update the PR with the documentation.

@yongchul yongchul Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vbarua I agree with the enum if there are more than two options but I don't see this particular one having the third options. The message does not have to be a collection of booleans. If a field has more than two options in the future, oh well, that field should be enum.

If you do want to see enum, please let me know!

@vbarua vbarua Dec 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with 2 options, I still have a strong preference for the enum version as it also makes it possible to check if the compatibility option has been set explicitly or not. I also find it's easier to document behaviour via the enum name. yield_no_rows_on_empty_input=true makes it clear that I shouldn't output a row, but yield_no_rows_on_empty_input=false isn't as explicit. How many rows should I output? What should be in the row, if anything. I would probably need to check the documentation.

If I see values like YIELD_EMPTY_ROW and YIELD_NO_ROW on the other hand, its very clear what I should be doing and I won't need to go look at the documentation.

@yongchul yongchul Dec 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vbarua In the proto or doc?

@yongchul yongchul Dec 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if this is a theme, we are effectively banning the usage of boolean in the Substrait, which is fine as this also happens in normal programming language -- passing 0 or boolean as function argument vs. enum. If we agree on, we should go over the spec and clean up all boolean and replace with enums in 1.0 perhaps.

@yongchul yongchul Dec 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vbarua changed to enum. PTAL!

@vbarua vbarua Dec 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are effectively banning the usage of boolean in the Substrait,

I would say heavily discouraging 😅

Aside from nullable, I don't think there's that many. But yes something to keep in mind for 1.0.

In the proto or doc?

I was thinking in the doc. This is surprisingly weird behaviour. I was testing with something like

SELECT COUNT(*), COUNT(id), SUM(id), STRING_AGG(s, ',')
FROM test;

on db-fiddle which outputs

(0, 0, null, null)

Trino does

trino> WITH test(i, s) AS (VALUES (1, 'a'), (2, 'b') LIMIT 0)
    -> SELECT COUNT(*), COUNT(i), SUM(i), LISTAGG(s) WITHIN GROUP (ORDER BY s) FROM test;
 _col0 | _col1 | _col2 | _col3
-------+-------+-------+-------
     0 |     0 |  NULL | NULL
(1 row)

So it looks like the behaviour on empty inputs is that the count functions return 0, and all other functions return null.

@yongchul yongchul Dec 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COUNT is special. All other aggregates are supposed to yield NULL over empty input.

}

// ConsistentPartitionWindowRel provides the ability to perform calculations across sets of rows
Expand Down
Loading