jayzhan211 commented on code in PR #26400:
URL: https://github.com/apache/datafusion/pull/26400#discussion_r4294453025
##########
datafusion/sql/src/select.rs:
##########
@@ -1437,42 +1437,106 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
{
let mut err = None;
let _ = visit_expressions_mut(expr, |expr| {
- if let SQLExpr::Function(f) = expr
- && let Some(WindowType::NamedWindow(ident)) = &f.over
- {
- let normalized_ident =
- self.ident_normalizer.normalize(ident.clone());
- for (
- NamedWindowDefinition(_, window_expr),
- normalized_window_ident,
- ) in named_windows.iter()
- {
- if normalized_ident.eq(normalized_window_ident) {
- f.over = Some(match window_expr {
- NamedWindowExpr::NamedWindow(ident) => {
- WindowType::NamedWindow(ident.clone())
- }
- NamedWindowExpr::WindowSpec(spec) => {
- WindowType::WindowSpec(spec.clone())
- }
- })
+ if let SQLExpr::Function(f) = expr {
Review Comment:
Only `select.projection` is resolved, so `OVER (w ...)` in QUALIFY and ORDER
BY still drops `w` silently. That is the same symptom as #26364:
```sql
SELECT ticker, ts, price FROM named_window_prices
QUALIFY MAX(price) OVER (w ROWS BETWEEN 1 PRECEDING AND CURRENT ROW) = price
WINDOW w AS (PARTITION BY ticker ORDER BY ts)
ORDER BY ticker, ts;
-- got: BTC 1, BTC 2, DOGE 2 expected: BTC 1, BTC 2, DOGE 1, DOGE 2
```
`ORDER BY ROW_NUMBER() OVER (w ROWS UNBOUNDED PRECEDING)` has the same
problem. Making the function generic over `VisitMut` fixes both and removes the
`SelectItem` match and the `err` option. Verified locally: the QUALIFY and
ORDER BY results above are correct, `QUALIFY ROW_NUMBER() OVER w = 1` now
plans, and the window/qualify slt files pass:
```diff
- query_order_by: Option<OrderBy>,
+ mut query_order_by: Option<OrderBy>,
@@
self.match_window_definitions(&mut select.projection,
&select.named_window)?;
+ self.match_window_definitions(&mut select.qualify,
&select.named_window)?;
+ self.match_window_definitions(&mut query_order_by,
&select.named_window)?;
```
```rs
fn match_window_definitions<V: VisitMut>(
&self,
node: &mut V,
named_windows: &[NamedWindowDefinition],
) -> Result<()> {
let named_windows: Vec<(&NamedWindowDefinition, String)> = named_windows
.iter()
.map(|w| (w, self.ident_normalizer.normalize(w.0.clone())))
.collect();
let flow = visit_expressions_mut(node, |expr| {
let SQLExpr::Function(f) = expr else {
return ControlFlow::Continue(());
};
let resolved = match &f.over {
Some(WindowType::NamedWindow(ident)) => {
self.resolve_named_window(ident, &named_windows)
}
Some(WindowType::WindowSpec(spec)) if spec.window_name.is_some()
=> {
self.resolve_window_spec(spec, &named_windows)
}
_ => return ControlFlow::Continue(()),
};
match resolved {
Ok(spec) => {
f.over = Some(WindowType::WindowSpec(spec));
ControlFlow::Continue(())
}
Err(e) => ControlFlow::Break(e),
}
});
match flow {
ControlFlow::Break(e) => Err(e),
ControlFlow::Continue(()) => Ok(()),
}
}
```
Please add a QUALIFY case and an ORDER BY case to `window.slt`.
##########
datafusion/sql/src/select.rs:
##########
@@ -1437,42 +1437,106 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
{
let mut err = None;
let _ = visit_expressions_mut(expr, |expr| {
- if let SQLExpr::Function(f) = expr
- && let Some(WindowType::NamedWindow(ident)) = &f.over
- {
- let normalized_ident =
- self.ident_normalizer.normalize(ident.clone());
- for (
- NamedWindowDefinition(_, window_expr),
- normalized_window_ident,
- ) in named_windows.iter()
- {
- if normalized_ident.eq(normalized_window_ident) {
- f.over = Some(match window_expr {
- NamedWindowExpr::NamedWindow(ident) => {
- WindowType::NamedWindow(ident.clone())
- }
- NamedWindowExpr::WindowSpec(spec) => {
- WindowType::WindowSpec(spec.clone())
- }
- })
+ if let SQLExpr::Function(f) = expr {
Review Comment:
`visit_expressions_mut` also descends into scalar subqueries, so a
subquery's `OVER (w ...)` resolves against the outer `WINDOW` clause:
```sql
SELECT (SELECT MAX(rn) FROM (
SELECT ROW_NUMBER() OVER (w ROWS UNBOUNDED PRECEDING) AS rn
FROM named_window_prices WINDOW w AS (PARTITION BY ticker ORDER BY ts)))
FROM named_window_prices WINDOW w AS (ORDER BY ts) LIMIT 1;
-- returns 6, expected 3; without the outer WINDOW: "The window w is not
defined!"
```
`OVER w` already behaves this way on main, so this is fine to handle in a
follow-up. A fix is to use a `VisitorMut` that counts depth in
`pre_visit_query` / `post_visit_query` and only rewrites at depth 0, so each
subquery resolves its own names in its own `select_to_plan`.
--
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]
