fix: fix aggregate expression outputs#181
Conversation
| let mut grouping_set: Vec<Value> = vec![]; | ||
| for i in &group.expression_references { | ||
| grouping_set.push(Value::Reference(*i as i32)); | ||
| let value = match rel.grouping_expressions.get(*i as usize) { |
There was a problem hiding this comment.
i is an index into rel.grouping_expressions, not a field reference into the input schema which is resolved to the actual expression rather than printing it as $i.
| index // is expression returned by this closure and inserted into map | ||
| }); | ||
| grouping_set.push(Value::Reference(expression_index_map[&key])); | ||
| if seen_expressions.insert(exp.encode_to_vec()) { |
There was a problem hiding this comment.
This is the same bug: Value::Reference(index) was replaced with Value::Expression(exp), the index-tracking machinery that value depended on became unused code.
wackywendell
left a comment
wackywendell
left a comment
There was a problem hiding this comment.
Generally pretty good! I have one small structural suggestion and a couple comment/docs ones, but pretty good!
| } | ||
|
|
||
| /// Parses an already-unwrapped inner pair of `Rule::expression`. | ||
| pub(crate) fn parse_expression_inner( |
There was a problem hiding this comment.
A couple things:
_inneris not a very good name for a function, it looks like its just the 'inside' of another function and doesn't really tell you what its doing.- By exporting an
_innerfunction from one module and importing it in another, the internal complexity of the implementations are being exposed. I think you can just directly useExpression::parse_pairon the outer (you may need to clone thePair, but aPairis a lightweight reference so that's fine), which makes it clearer where its called what is happening and preserves the interface boundaries.
Co-authored-by: Wendell Smith <wackywendell@gmail.com>
….com:DataDog/substrait-explain into gordon.hamilton/aggregate-expression-outputs
There was a problem hiding this comment.
Pull request overview
Fixes incorrect handling of Aggregate relation outputs across both parsing and textification so that grouping-expression outputs resolve to the correct grouping positions/expressions (rather than being misinterpreted as measures or rendered as raw $i indexes). This aligns the text format more closely with Substrait’s AggregateRel output schema semantics (grouping_expressions + measures) and improves roundtrip correctness.
Changes:
- Parser: change
aggregate_outputparsing to accept generalexpressions, mapping outputs back to grouping-expression positions by structural equality and only treating non-matchingfunction_calls as new measures. - Textifier: fix grouping-set rendering to print the actual grouping expressions (resolving
expression_referencesthroughgrouping_expressions), including the deprecated per-Grouping code path. - Docs/tests: update grammar documentation and add regression tests for reordered groupings, non-function output expressions, and error messaging.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/textify/rels.rs | Fixes grouping-set rendering to print resolved grouping expressions (including deprecated grouping fields) and adds regression tests. |
| src/parser/relations.rs | Reworks aggregate output parsing to map outputs to grouping positions by structural equality and adds tests for correct mapping and errors. |
| src/parser/expressions.rs | Updates aggregate-measure parsing to parse directly from function_call after grammar changes. |
| src/parser/expression_grammar.pest | Broadens aggregate_output grammar from `(reference |
| GRAMMAR.md | Updates user-facing grammar/docs to reflect new aggregate output rules and measure syntax limitations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Fix Aggregate output/grouping-expression handling
Aggregate relation output items were mishandled in two ways:
Type of Change
Testing
Related Issues
Closes #175