You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "cdelmonte-zg (via GitHub)" <gi...@apache.org> on 2026/08/28 10:46:14 UTC
[PR] fix: prevent CSE from extracting window functions [datafusion]
cdelmonte-zg opened a new pull request, #24750:
URL: https://github.com/apache/datafusion/pull/24750
## Which issue does this PR close?
- Closes #15190
## Rationale for this change
Common subexpression elimination can extract duplicated window functions into an intermediate Projection. Since window functions cannot be evaluated by a projection, this produces a plan that fails during physical planning.
## What changes are included in this PR?
Exclude WindowFunction expressions themselves from CSE extraction and add a regression test.
## Are these changes tested?
Yes. A regression test verifies that duplicated window functions remain in the WindowAggr node.
## Are there any user-facing changes?
Well, queries affected by #15190 no longer fail during physical planning, no changes in docs etc tho
--
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: prevent CSE from extracting window functions [datafusion]
Posted by "cdelmonte-zg (via GitHub)" <gi...@apache.org>.
cdelmonte-zg commented on PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#issuecomment-5521772943
@alamb could you please trigger the CI once again? I added a regression test following the review suggestion. Thanks!
--
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: prevent CSE from extracting window functions [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#issuecomment-5486843344
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24750?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 `83.33333%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.56%. Comparing base ([`a274959`](https://app.codecov.io/gh/apache/datafusion/commit/a2749598bea2e65241fdbf011a4aac95b58079a7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`85115d2`](https://app.codecov.io/gh/apache/datafusion/commit/85115d26b087cdfbe4f768b4adb70065d40f45c4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24750?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...tafusion/optimizer/src/common\_subexpr\_eliminate.rs](https://app.codecov.io/gh/apache/datafusion/pull/24750?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Fcommon_subexpr_eliminate.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL2NvbW1vbl9zdWJleHByX2VsaW1pbmF0ZS5ycw==) | 83.33% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24750?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 #24750 +/- ##
==========================================
- Coverage 81.58% 81.56% -0.02%
==========================================
Files 1123 1123
Lines 406610 406622 +12
Branches 406610 406622 +12
==========================================
- Hits 331719 331675 -44
- Misses 55453 55505 +52
- Partials 19438 19442 +4
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24750?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: prevent CSE from extracting window functions [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#issuecomment-5535563873
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v55.0.0 (current)
Built [ 59.371s] (current)
Parsing datafusion v55.0.0 (current)
Parsed [ 0.035s] (current)
Building datafusion v55.0.0 (baseline)
Built [ 59.442s] (baseline)
Parsing datafusion v55.0.0 (baseline)
Parsed [ 0.035s] (baseline)
Checking datafusion v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.656s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure method_requires_different_generic_type_params: method now requires a different number of generic type parameters ---
Description:
A method now requires a different number of generic type parameters than it used to. Uses of this method that supplied the previous number of generic types will be broken.
ref: https://doc.rust-lang.org/reference/items/generics.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/method_requires_different_generic_type_params.ron
Failed in:
datafusion::prelude::dataframe::DataFrame::from_columns takes 0 generic types instead of 1, in /home/runner/work/datafusion/datafusion/datafusion/core/src/dataframe/mod.rs:2631
datafusion::dataframe::DataFrame::from_columns takes 0 generic types instead of 1, in /home/runner/work/datafusion/datafusion/datafusion/core/src/dataframe/mod.rs:2631
datafusion::prelude::DataFrame::from_columns takes 0 generic types instead of 1, in /home/runner/work/datafusion/datafusion/datafusion/core/src/dataframe/mod.rs:2631
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 121.609s] datafusion
Building datafusion-optimizer v55.0.0 (current)
Built [ 27.896s] (current)
Parsing datafusion-optimizer v55.0.0 (current)
Parsed [ 0.029s] (current)
Building datafusion-optimizer v55.0.0 (baseline)
Built [ 27.702s] (baseline)
Parsing datafusion-optimizer v55.0.0 (baseline)
Parsed [ 0.031s] (baseline)
Checking datafusion-optimizer v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.156s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/inherent_method_missing.ron
Failed in:
TypeCoercionRewriter::coerce_asof_join, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/9c071b6a594f5a726eea3c3d7142a63e6c14f674/datafusion/optimizer/src/analyzer/type_coercion.rs:289
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 56.771s] datafusion-optimizer
```
</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: prevent CSE from extracting window functions [datafusion]
Posted by "cdelmonte-zg (via GitHub)" <gi...@apache.org>.
cdelmonte-zg commented on PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#issuecomment-5467483849
@alamb could you please trigger the CI for this PR? Thanks.
--
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: prevent CSE from extracting window functions [datafusion]
Posted by "cdelmonte-zg (via GitHub)" <gi...@apache.org>.
cdelmonte-zg commented on code in PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#discussion_r3912250064
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -732,6 +732,7 @@ impl CSEController for ExprCSEController<'_> {
| Expr::Wildcard { .. }
| Expr::Lambda(_)
| Expr::LambdaVariable(_)
+ | Expr::WindowFunction(..)
Review Comment:
Thank you, happy that it works!
I followed your suggestion, but the SQL planner deduplicates identical window expressions before they reach CSE, so the SQLLogicTest could not reproduce the bug.
I added an execution-level regression test, tho. Is that okay?
--
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: prevent CSE from extracting window functions [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24750:
URL: https://github.com/apache/datafusion/pull/24750
--
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: prevent CSE from extracting window functions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#issuecomment-5590816846
Thanks -- this is a nice fix @cdelmonte-zg and thanks (as always) for the reviews @kosiew
--
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: prevent CSE from extracting window functions [datafusion]
Posted by "cdelmonte-zg (via GitHub)" <gi...@apache.org>.
cdelmonte-zg commented on PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#issuecomment-5553969839
@kosiew thank you for your review and assistance! Glad to have contributed :)
--
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: prevent CSE from extracting window functions [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#discussion_r3905041245
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -732,6 +732,7 @@ impl CSEController for ExprCSEController<'_> {
| Expr::Wildcard { .. }
| Expr::Lambda(_)
| Expr::LambdaVariable(_)
+ | Expr::WindowFunction(..)
Review Comment:
Nice fix. Would it be worth adding an end-to-end SQLLogicTest that executes two identical window expressions over non-empty input, ideally with `PARTITION BY` and `ORDER BY`? The current unit test does a good job of verifying that CSE does not introduce an intermediate projection, but since the original failure happens during physical planning, an execution-level regression test would give us coverage across that boundary as well.
I noticed the existing `window.slt` case that uses a window expression in both `SELECT` and `ORDER BY`, but that seems to exercise a slightly different path since it resolves to a single physical window expression before CSE. This is just a suggestion and not a blocker.
--
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: prevent CSE from extracting window functions [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#issuecomment-5551145058
@cdelmonte-zg
> I saw that the semver is reporting a version incompatibility of pr and main branch. Should I rebase or is it not necessary at this point?
I think it is fine to merge as is.
🚀
Thank you for your contribution.
--
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: prevent CSE from extracting window functions [datafusion]
Posted by "cdelmonte-zg (via GitHub)" <gi...@apache.org>.
cdelmonte-zg commented on PR #24750:
URL: https://github.com/apache/datafusion/pull/24750#issuecomment-5536118273
Hi @kosiew , thank you for retriggering the ci pipeline! I have just a question: I saw that the semver is reporting a version incompatibility of pr and main branch. Should I rebase or is it not necessary at this point? Tnx
--
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