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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
