Skip to content

fix: preserve ExprFunctionExt options when chaining - #26486

Draft
goutamadwant wants to merge 2 commits into
apache:mainfrom
goutamadwant:fix-25934-window-frame-options
Draft

goutamadwant wants to merge 2 commits into
apache:mainfrom
goutamadwant:fix-25934-window-frame-options

Conversation

@goutamadwant

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Adding an option to an existing aggregate or window expression can silently reset options already set on that expression. For example, adding DISTINCT can discard ORDER BY and FILTER; adding a window partition can discard ordering. An implicit window frame must also be re-derived when ORDER BY changes, while an explicit frame must remain unchanged.

What changes are included in this PR?

  • Seed ExprFuncBuilder from existing aggregate/window options and derive the default window frame from the final ordering.
  • Track whether a window frame was explicit through expression construction, SQL planning/unparsing, protobuf, and Substrait where representable. Keep this provenance bit out of expression equality, ordering, and hashing.
  • Add regression and round-trip tests, plus an upgrade guide note for the new public fields.

Substrait cannot retain implicit-frame provenance for ordered default frames while preserving their execution behavior; those frames need explicit bounds on export. Older protobuf plans decode the new flag as false while retaining the resolved frame.

What is the testing strategy for this PR?

  • Core expression/API tests: 43 passed, covering option chaining, frame provenance, and equality/hash behavior.
  • SQL round-trip and unparser tests: 47 and 38 passed.
  • Protobuf and Substrait window round-trip tests: 3 and 1 passed.
  • Rust formatting and documentation Prettier checks passed. Full-workspace Clippy passed with clippy::mutable_key_type allowed; the unmodified command reports that lint in existing HashSet<&Expr> uses in datafusion/optimizer/src/push_down_filter.rs, which this PR does not change.

These are focused local tests; CI has not run for this branch.

Are there any user-facing changes?

Yes. Chained aggregate/window options are preserved, and implicit window frames update when ordering changes. WindowFunctionParams and RawWindowExpr gain a public window_frame_explicit field, so callers constructing these structs with literals must set it. The 56.0.0 upgrade guide documents this API change; please apply the api change label.

@github-actions github-actions Bot added documentation Improvements or additions to documentation sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate substrait Changes to the substrait crate proto Related to proto crate functions Changes to functions implementation labels Oct 18, 2026
@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details
     Cloning apache/main
    Building datafusion v55.1.0 (current)
       Built [  54.321s] (current)
     Parsing datafusion v55.1.0 (current)
      Parsed [   0.033s] (current)
    Building datafusion v55.1.0 (baseline)
       Built [  54.504s] (baseline)
     Parsing datafusion v55.1.0 (baseline)
      Parsed [   0.033s] (baseline)
    Checking datafusion v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.632s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [ 111.782s] datafusion
    Building datafusion-expr v55.1.0 (current)
       Built [  29.272s] (current)
     Parsing datafusion-expr v55.1.0 (current)
      Parsed [   0.087s] (current)
    Building datafusion-expr v55.1.0 (baseline)
       Built [  29.469s] (baseline)
     Parsing datafusion-expr v55.1.0 (baseline)
      Parsed [   0.090s] (baseline)
    Checking datafusion-expr v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   1.364s] 229 checks: 228 pass, 1 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field WindowFunctionParams.window_frame_explicit in /home/runner/work/datafusion/datafusion/datafusion/expr/src/expr.rs:1264
  field RawWindowExpr.window_frame_explicit in /home/runner/work/datafusion/datafusion/datafusion/expr/src/planner.rs:360

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  62.286s] datafusion-expr
    Building datafusion-functions-window v55.1.0 (current)
       Built [  23.886s] (current)
     Parsing datafusion-functions-window v55.1.0 (current)
      Parsed [   0.011s] (current)
    Building datafusion-functions-window v55.1.0 (baseline)
       Built [  23.855s] (baseline)
     Parsing datafusion-functions-window v55.1.0 (baseline)
      Parsed [   0.011s] (baseline)
    Checking datafusion-functions-window v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.094s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  48.794s] datafusion-functions-window
    Building datafusion-optimizer v55.1.0 (current)
       Built [  24.157s] (current)
     Parsing datafusion-optimizer v55.1.0 (current)
      Parsed [   0.035s] (current)
    Building datafusion-optimizer v55.1.0 (baseline)
       Built [  24.562s] (baseline)
     Parsing datafusion-optimizer v55.1.0 (baseline)
      Parsed [   0.036s] (baseline)
    Checking datafusion-optimizer v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.174s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  49.667s] datafusion-optimizer
    Building datafusion-proto v55.1.0 (current)
       Built [  49.850s] (current)
     Parsing datafusion-proto v55.1.0 (current)
      Parsed [   0.013s] (current)
    Building datafusion-proto v55.1.0 (baseline)
       Built [  49.538s] (baseline)
     Parsing datafusion-proto v55.1.0 (baseline)
      Parsed [   0.013s] (baseline)
    Checking datafusion-proto v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.146s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [ 100.997s] datafusion-proto
    Building datafusion-proto-models v55.1.0 (current)
       Built [  22.691s] (current)
     Parsing datafusion-proto-models v55.1.0 (current)
      Parsed [   0.143s] (current)
    Building datafusion-proto-models v55.1.0 (baseline)
       Built [  22.869s] (baseline)
     Parsing datafusion-proto-models v55.1.0 (baseline)
      Parsed [   0.143s] (baseline)
    Checking datafusion-proto-models v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   2.018s] 229 checks: 228 pass, 1 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field WindowExprNode.window_frame_explicit in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1149
  field WindowExprNode.window_frame_explicit in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1149

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  48.918s] datafusion-proto-models
    Building datafusion-sql v55.1.0 (current)
       Built [  40.162s] (current)
     Parsing datafusion-sql v55.1.0 (current)
      Parsed [   0.030s] (current)
    Building datafusion-sql v55.1.0 (baseline)
       Built [  40.373s] (baseline)
     Parsing datafusion-sql v55.1.0 (baseline)
      Parsed [   0.032s] (baseline)
    Checking datafusion-sql v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.258s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  82.453s] datafusion-sql
    Building datafusion-substrait v55.1.0 (current)
       Built [ 306.025s] (current)
     Parsing datafusion-substrait v55.1.0 (current)
      Parsed [   0.011s] (current)
    Building datafusion-substrait v55.1.0 (baseline)
       Built [ 308.670s] (baseline)
     Parsing datafusion-substrait v55.1.0 (baseline)
      Parsed [   0.015s] (baseline)
    Checking datafusion-substrait v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.216s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [ 616.926s] datafusion-substrait

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Oct 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change core Core DataFusion crate documentation Improvements or additions to documentation functions Changes to functions implementation logical-expr Logical plan and expressions optimizer Optimizer rules proto Related to proto crate sql SQL Planner substrait Changes to the substrait crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Record whether a window frame was set explicitly, so ExprFunctionExt can build on an existing window function

1 participant