Implement Substrait Support for GROUPING SET CUBE
#18798
Merged
Conversation
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
Remove NotImplemented error for GroupingSet::Cube to allow Substrait conversion. Add the generate_powerset function for efficient power set generation using bit-masking. Include validation to manage memory usage and refactor error handling for consistency across GroupingSet variants. Add tests for CUBE queries and validate with SQLLogicTest.
…or index iteration
github-actions
bot
added
logical-expr
Contributor
Author
|
@martin-g |
Contributor
Author
|
@gabotechs |
gabotechs
approved these changes
Nov 24, 2025
Contributor
gabotechs
left a comment
gabotechs
left a comment
There was a problem hiding this comment.
👍 nice. Left a suggestion about reducing even more the necessary changes, but otherwise LGTM
Comment on lines
97
to
108
| let cube_sets = powerset_cloned(set)?; | ||
| cube_sets | ||
| .iter() | ||
| .map(|set| { | ||
| parse_flat_grouping_exprs( | ||
| producer, | ||
| set, | ||
| schema, | ||
| &mut ref_group_exprs, | ||
| ) | ||
| }) | ||
| .collect::<datafusion::common::Result<Vec<_>>>() |
Contributor
There was a problem hiding this comment.
Any opinions on just using the normal powerset() function here instead of creating a *_cloned() variant? handling the clone here should be trivial:
- let cube_sets = powerset_cloned(set)?;
+ let cube_sets = powerset(set)?;
cube_sets
.iter()
.map(|set| {
parse_flat_grouping_exprs(
producer,
- set,
+ &set.iter().map(|v| (*v).clone()).collect::<Vec<_>>(),
schema,
&mut ref_group_exprs,
)
})And that would avoid maintaining 3 function signatures (powerset, powerset_cloned and powerset_indices)
…nce in grouping expressions
…ble from the substrait module.
Contributor
Author
|
@gabotechs |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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?
Rationale for this change
This change adds complete Substrait round‑trip support for
GROUPING SET CUBE, allowing logical plans containing cubes to successfully convert to and from Substrait. Previously, cube handling in the Substrait producer returned a hardNotImplementederror, preventing several SQLLogicTest cases from running under round‑trip mode.Supporting
CUBEbrings consistency with existingROLLUPandGROUPING SETShandling, ensures correct logical plan serialization, and enables successful execution of tests that rely on cube semantics.Before
After
What changes are included in this PR?
powerset_indicesfor efficient subset generation.powersetto use DataFusion error types and removes the string‑based error.powerset_clonedfunction for owned‑value subsets needed in the Substrait adapter.GroupingSet::Cubeusingpowerset_cloned.GROUP BY CUBE.Are these changes tested?
Yes. A new Substrait round‑trip test (
aggregate_grouping_cube) validates the logical plan after translation. Existing grouping/aggregate tests continue to pass, covering other grouping‑set variants.Are there any user-facing changes?
There are no user‑facing API changes. The behavior of
GROUP BY CUBEis now consistent under Substrait round‑trip mode, which may allow previously failing queries to succeed.LLM-generated code disclosure
This PR includes LLM‑generated code and comments. All LLM‑generated content has been manually reviewed and tested.