Skip to content

fix: reject unsupported Substrait aggregation phases - #25045

Open
goutamadwant wants to merge 1 commit into
apache:mainfrom
goutamadwant:fix-substrait-aggregate-phase-24967
Open

fix: reject unsupported Substrait aggregation phases#25045
goutamadwant wants to merge 1 commit into
apache:mainfrom
goutamadwant:fix-substrait-aggregate-phase-24967

Conversation

@goutamadwant

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

The Substrait consumer treats intermediate aggregate phases as complete calls. This can silently return final values when a plan requests intermediate state, or report an unrelated root-schema naming error.

What changes are included in this PR?

  • Validate phases in aggregate and window expressions before translating their arguments.
  • Accept INITIAL_TO_RESULT and retain UNSPECIFIED for compatibility with existing DataFusion-produced plans.
  • Reject intermediate phases and unknown protobuf enum values with explicit errors.

What is the testing strategy for this PR?

  • Reproduced the original behavior: an INITIAL_TO_INTERMEDIATE average over values 1 and 2 returned 1.5 instead of intermediate state.
  • Added tests for supported phases, every unsupported phase, rooted and unrooted aggregates, unknown binary-protobuf enum values, and actual window output.
  • The full Substrait integration target passes: 213 tests passed, with six existing tests ignored.
  • The required extended workspace suite passes: 11,267 tests passed, eight existing tests ignored, and all 511 SQL logic-test files completed.
  • cargo clippy --all-targets --all-features -- -D warnings passes.
  • The complete ./dev/rust_lint.sh passes, including strict workspace documentation checks.

Are there any user-facing changes?

Plans with unsupported aggregate or window phases now fail explicitly instead of being interpreted as complete calls. Existing unspecified-phase plans remain accepted. No public Rust API changes are included. Intermediate-state execution and the separate AVG output-type mismatch are not addressed here.

@github-actions github-actions Bot added the substrait Changes to the substrait crate label Sep 7, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.72%. Comparing base (1b6dc92) to head (6e8b7b3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25045      +/-   ##
==========================================
- Coverage   81.72%   81.72%   -0.01%     
==========================================
  Files        1127     1127              
  Lines      416310   416318       +8     
  Branches   416310   416318       +8     
==========================================
+ Hits       340224   340226       +2     
- Misses      56095    56103       +8     
+ Partials    19991    19989       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kumarUjjawal

Copy link
Copy Markdown
Contributor

Hi @goutamadwant thank you for the fix, though I see alexandrefimov mentioned opening a PR. Can you please coordinate with the issue author so we don't duplicate the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

substrait Changes to the substrait crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Substrait consumer ignores AggregateFunction.phase, so an intermediate aggregate runs as a complete one

3 participants