You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "anishmehta24 (via GitHub)" <gi...@apache.org> on 2026/09/19 20:23:01 UTC
[PR] fix: honor the volatility of aggregate, window and higher-order functions in `Expr::is_volatile` [datafusion]
anishmehta24 opened a new pull request, #25513:
URL: https://github.com/apache/datafusion/pull/25513
## Which issue does this PR close?
- Closes #25504.
## 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](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: honor the volatility of aggregate, window and higher-order functions in `Expr::is_volatile` [datafusion]
Posted by "anishmehta24 (via GitHub)" <gi...@apache.org>.
anishmehta24 commented on PR #25513:
URL: https://github.com/apache/datafusion/pull/25513#issuecomment-5750251021
Closing in favour of #25511, which was opened first and covers the same change (signature volatility for aggregate, window and higher-order functions, including aggregates used as window functions) with an end-to-end regression on top. Sorry for the duplicate; I should have checked for an existing PR by title and used `take` on the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: honor the volatility of aggregate, window and higher-order functions in `Expr::is_volatile` [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25513:
URL: https://github.com/apache/datafusion/pull/25513#issuecomment-5747336239
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25513?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `60.17699%` with `45 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.38%. Comparing base ([`710c748`](https://app.codecov.io/gh/apache/datafusion/commit/710c748db8931bbe28ade5f25436d0672a19b355?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c225aae`](https://app.codecov.io/gh/apache/datafusion/commit/c225aae8ece9654a70f23ab93d7ac8b9ab5e81aa?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25513?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/expr/src/expr.rs](https://app.codecov.io/gh/apache/datafusion/pull/25513?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Fexpr.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9leHByLnJz) | 60.17% | [45 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25513?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25513?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: honor the volatility of aggregate, window and higher-order functions in `Expr::is_volatile` [datafusion]
Posted by "anishmehta24 (via GitHub)" <gi...@apache.org>.
anishmehta24 closed pull request #25513: fix: honor the volatility of aggregate, window and higher-order functions in `Expr::is_volatile`
URL: https://github.com/apache/datafusion/pull/25513
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org