Repository navigation
fix(proto): keep FILTER, IGNORE NULLS and reversal state of window expressions in physical plans - #27207
Open
shehabgamin wants to merge 1 commit into
Open
fix(proto): keep FILTER, IGNORE NULLS and reversal state of window expressions in physical plans#27207shehabgamin wants to merge 1 commit into
shehabgamin wants to merge 1 commit into
Conversation
…pressions in physical plans
github-actions
Bot
added
physical-expr
shehabgamin
marked this pull request as ready for review
November 7, 2026 14:32
shehabgamin
changed the title
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the window-expression protobuf round-trip changes.
No open issues 🎉
What changed in this PR
This PR addresses #27206 by preserving window-expression state during physical-plan protobuf round trips, preventing serialization from changing query results.
Changes:
- Preserve aggregate
FILTER,IGNORE NULLS, reversal state, and aggregate ordering. - Consolidate window-expression encoding and decoding in
datafusion-physical-plan. - Add round-trip regression tests and regenerate protobuf and JSON bindings.
| File | Description |
|---|---|
datafusion/proto/tests/cases/plans/windows.rs |
Adds tests for filters, null handling, reversal, and aggregate ordering. |
datafusion/proto/src/physical_plan/to_proto.rs |
Delegates window serialization to the shared encoder. |
datafusion/proto/src/physical_plan/from_proto.rs |
Delegates window deserialization to the shared decoder. |
datafusion/proto-models/src/generated/prost.rs |
Adds generated fields and boxes the recursive window-expression variant. |
datafusion/proto-models/src/generated/pbjson.rs |
Adds JSON serialization and deserialization for the new fields. |
datafusion/proto-models/proto/datafusion.proto |
Defines filter, reversal, and aggregate-ordering fields. |
datafusion/physical-plan/src/windows/proto.rs |
Encodes and restores window-expression state through shared public helpers. |
datafusion/physical-plan/src/windows/mod.rs |
Exposes codec helpers and state accessors; supports restoring reversed window functions. |
datafusion/physical-expr/src/window/mod.rs |
Re-exports AggregateWindowExpr for access to aggregate filters. |
Review effort: Balanced
40 of 61 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Rationale for this change
A physical plan protobuf round trip can change the results of window queries. The window aggregate
FILTERis dropped.IGNORE NULLSon window functions is dropped. Window functions that the optimizer reversed are decoded as not reversed, solagreturns the following row. The ordering of an aggregate used as a window function is also dropped.What changes are included in this PR?
The patch adds
filter,is_reversedandordering_reqtoPhysicalWindowExprNodeand regenerates the proto code. The window encoder now writes the realignore_nulls,is_reversed, aggregate ordering and filter, and the decoder rebuilds the window expression with them.serialize_physical_window_expr/parse_physical_window_exprnow delegate to the same encoder and decoder indatafusion-physical-planinstead of keeping a second copy.What is the testing strategy for this PR?
Three new round-trip tests in
datafusion/proto/tests/cases/plans/windows.rseach fail without this change:roundtrip_window_filter: plain and sliding window aggregates withFILTERroundtrip_window_udf_ignore_nulls_and_reversed:lagwithIGNORE NULLS, and a reversedlagroundtrip_window_aggregate_ordering: an orderedarray_aggwindow aggregate and its reversed formAre there any user-facing changes?
Window expressions now survive physical plan serialization. API changes:
AggregateWindowExpr,WindowUDFExpr::ignore_nulls()/is_reversed(), andencode_physical_window_expr/decode_physical_window_expr.physical_expr_node::ExprType::WindowExprvariant, so it holdsBox<PhysicalWindowExprNode>.