feat: implement window functions - #188
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02163d48b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if is_list_of_fields { | ||
| items | ||
| .into_iter() | ||
| .map(|item| sort_field_from_tuple(extensions, unwrap_single_pair(item))) |
There was a problem hiding this comment.
If a window order= list starts with a valid sort tuple but contains a later non-tuple item, e.g. order=(($0,&AscNullsLast), $1), is_list_of_fields is true and this call passes a reference pair into sort_field_from_tuple, whose assert_eq!(tuple.as_rule(), Rule::tuple) panics instead of returning a MessageParseError. This is syntactically parseable bad input, so the parser should validate every item and report the contextual error rather than aborting the caller.
AGENTS.md reference: AGENTS.md:L54-L56
Useful? React with 👍 / 👎.
| ) | ||
| })?; | ||
|
|
||
| let expr = window_expression_from_value(extensions, expr_pair)?; |
There was a problem hiding this comment.
Using window_expression_from_value here lets order=(add($0):i64, &AscNullsLast) parse into a SortField with a scalar-function expression, but the shared formatter for SortField only renders selection references and emits !{SortField} for any other expression. Since the documented window syntax describes the sort key as a reference, this accepted input cannot be formatted back into parseable text; either reject non-reference order keys here or teach the formatter to render them.
AGENTS.md reference: AGENTS.md:L80-L83
Useful? React with 👍 / 👎.
02163d4 to
53eea56
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for Substrait Expression.WindowFunction in substrait-explain, extending the text format, parser, and textifier so window functions can be round-tripped through parse -> textify -> parse.
Changes:
- Adds parsing for
window_function_callexpressions with anover(...)clause (partition/order/phase/invocation + optional rows/range frame) and the necessary semantic validations. - Adds textification for
WindowFunction, including canonical rendering ofover(...)named arguments and frame bounds. - Updates the Pest grammar and
GRAMMAR.md, and adds roundtrip + rejection tests for window function behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/plan_roundtrip.rs | Adds roundtrip cases and plan-level rejection cases for window function syntax/semantics. |
| src/textify/expressions.rs | Implements Textify for WindowFunction and supporting helpers for over(...) arguments and frame rendering. |
| src/parser/extensions.rs | Explicitly rejects _ as an extension-relation argument value now that _ is syntactically allowed as an argument token. |
| src/parser/expressions.rs | Implements window function parsing (window_function_call) and over(...) clause decoding/validation. |
| src/parser/expression_grammar.pest | Introduces window_function_call grammar and allows _ (empty) as an extension_argument for frame bounds. |
| GRAMMAR.md | Documents the new window function syntax and clarifies _ as a syntactic argument value with limited semantic meaning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let phase_pair = phase_pair.ok_or_else(|| { | ||
| MessageParseError::invalid( | ||
| "WindowFunction", | ||
| span, | ||
| "Missing required phase= argument in over(...)", | ||
| ) | ||
| })?; | ||
| let phase = match window_enum_str(phase_pair, "phase")?.as_str() { | ||
| "Unspecified" => AggregationPhase::Unspecified as i32, | ||
| "InitialToIntermediate" => AggregationPhase::InitialToIntermediate as i32, | ||
| "IntermediateToIntermediate" => AggregationPhase::IntermediateToIntermediate as i32, | ||
| "InitialToResult" => AggregationPhase::InitialToResult as i32, | ||
| "IntermediateToResult" => AggregationPhase::IntermediateToResult as i32, | ||
| other => { | ||
| return Err(MessageParseError::invalid( | ||
| "AggregationPhase", | ||
| span, | ||
| format!("Unknown AggregationPhase: {other}"), | ||
| )); | ||
| } | ||
| }; |
| let invocation = match invocation_pair { | ||
| None => AggregationInvocation::Unspecified as i32, | ||
| Some(p) => match window_enum_str(p, "invocation")?.as_str() { | ||
| "Unspecified" => AggregationInvocation::Unspecified as i32, | ||
| "All" => AggregationInvocation::All as i32, | ||
| "Distinct" => AggregationInvocation::Distinct as i32, | ||
| other => { | ||
| return Err(MessageParseError::invalid( | ||
| "AggregationInvocation", | ||
| span, | ||
| format!("Unknown AggregationInvocation: {other}"), | ||
| )); | ||
| } | ||
| }, | ||
| }; |
e0b8d95 to
c297ab1
Compare
gord02
changed the title
6836dd4 to
8b9bac6
Compare
Add parsing and textifying of window functions (Expression.WindowFunction): an `over(...)` clause with partition=, order=, phase=, invocation=, and rows=/range= frame bounds. Reuses the generic argument/tuple/named-argument grammar (adding '_' to extension_argument for unbounded bounds, rejected as an extension-relation argument), composes the shared FunctionReference and FunctionArguments parser components into WindowFunctionInvocation/OverClause/Bound, and textifies via the shared value primitives. Includes GRAMMAR.md docs and parser + roundtrip tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8b9bac6 to
acfc254
Compare
Description
This PR implements window function building on the refactoring PRs.
Type of Change
Testing
Related Issues
Closes #158