Skip to content

fix: honor the volatility of aggregate, window and higher-order functions in Expr::is_volatile - #25513

Open
anishmehta24 wants to merge 1 commit into
apache:mainfrom
anishmehta24:fix/expr-is-volatile-udaf-udwf-hof
Open

anishmehta24 wants to merge 1 commit into
apache:mainfrom
anishmehta24:fix/expr-is-volatile-udaf-udwf-hof

Conversation

@anishmehta24

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

Expr::is_volatile_node only matched Expr::ScalarFunction, so Expr::is_volatile never saw the volatility that an AggregateUDF, WindowUDF or HigherOrderUDF declares in its signature. Every optimizer rule built on is_volatile (common subexpression elimination, filter pushdown, TopK-through-join, the simplifier, is_repeatable in optimizer::utils) therefore treated a volatile user-defined function of those kinds as deterministic and could deduplicate, move or double-evaluate it. The physical HigherOrderFunctionExpr::is_volatile_node already checks its signature; the logical Expr was the outlier.

What changes are included in this PR?

Expr::is_volatile_node now reads the signature volatility of Expr::ScalarFunction, Expr::AggregateFunction, Expr::WindowFunction (both WindowFunctionDefinition variants, so an aggregate used as a window function keeps its volatility) and Expr::HigherOrderFunction. Every other node is still non-volatile. Expr::is_volatile is unchanged and picks the new cases up through exists.

What is the testing strategy for this PR?

New unit test expr::test::test_is_volatile_aggregate_window_and_higher_order_funcs in datafusion/expr/src/expr.rs: for Immutable, Stable and Volatile signatures it builds an aggregate, a window aggregate, a window UDF and a higher-order function call and checks is_volatile_node / is_volatile, plus the nested lit + agg case. The aggregate/window/HOF assertions fail on main. All built-in functions in these categories are Immutable, so the existing sqllogictests are unaffected.

Are there any user-facing changes?

Optimizer rules now leave volatile user-defined aggregate, window and higher-order functions alone, as they already did for volatile scalar functions. No API change.

🤖 Generated with Claude Code

…ions in `Expr::is_volatile`

`Expr::is_volatile_node` only looked at `Expr::ScalarFunction`, so a
volatile `AggregateUDF`, `WindowUDF` or `HigherOrderUDF` looked
deterministic to `Expr::is_volatile` and to every optimizer rule built on
it (common subexpression elimination, filter and TopK pushdown, the
simplifier). The physical `HigherOrderFunctionExpr::is_volatile_node`
already checked the signature; the logical `Expr` was the outlier.

Closes apache#25504
@github-actions github-actions Bot added the logical-expr Logical plan and expressions label Sep 19, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.17699% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.38%. Comparing base (710c748) to head (c225aae).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/expr/src/expr.rs 60.17% 45 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25513      +/-   ##
==========================================
- Coverage   82.38%   82.38%   -0.01%     
==========================================
  Files        1138     1138              
  Lines      434313   434421     +108     
  Branches   434313   434421     +108     
==========================================
+ Hits       357805   357879      +74     
- Misses      54876    54908      +32     
- Partials    21632    21634       +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.

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

Labels

logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expr::is_volatile ignores the volatility of aggregate, window and higher-order functions

2 participants