Repository navigation
Resolve named windows referenced inside OVER (...) - #26400
Conversation
A window such as OVER (w ROWS BETWEEN ...) referenced the named window w inside its parentheses, but the planner only resolved OVER w and ignored the name, so the window lost w's PARTITION BY and ORDER BY and ran over all rows. Copy the referenced window's PARTITION BY and ORDER BY and apply the PostgreSQL rules for what may be added, so a window definition can also reference an earlier one.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26400 +/- ##
==========================================
+ Coverage 82.87% 82.89% +0.01%
==========================================
Files 1153 1154 +1
Lines 457065 458072 +1007
Branches 457065 458072 +1007
==========================================
+ Hits 378800 379721 +921
- Misses 55253 55308 +55
- Partials 23012 23043 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @zhuqi-lucas , here are some suggestions
| WindowType::WindowSpec(spec.clone()) | ||
| } | ||
| }) | ||
| if let SQLExpr::Function(f) = expr { |
There was a problem hiding this 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:
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 2ORDER 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:
- 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)?;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.
There was a problem hiding this comment.
visit_expressions_mut also descends into scalar subqueries, so a subquery's OVER (w ...) resolves against the outer WINDOW clause:
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.
There was a problem hiding this comment.
Thanks @jayzhan211, good catch! Applied in 2d171c5: QUALIFY and ORDER BY are resolved as well, with QUALIFY and ORDER BY cases added to window.slt.
There was a problem hiding this comment.
Thanks! Fixed it in this PR rather than a follow-up: the resolver now tracks query depth and only rewrites at depth 0, so each subquery resolves against its own WINDOW clause. Added your case, plus one where the subquery has no WINDOW clause, which now errors like PostgreSQL.
|
While reviewing this PR I found a related problem with named windows in |
|
Thanks @jayzhan211 and @alamb for review! |
## Which issue does this PR close? - Closes apache#26506. ## Rationale for this change A named window in `HAVING` was never resolved, so `MAX(price) OVER w` was planned as a plain aggregate and the query returned rows instead of being rejected. An undefined window name in `HAVING` was not reported either. ## What changes are included in this PR? Resolve named windows in `HAVING` together with the other clauses. The query now fails like the inline form (`Window function calls are not allowed in HAVING`), and an undefined name reports `The window ... is not defined!`. This builds on apache#26400, which makes the resolver usable for any clause. Only the last commit is new; the diff shrinks to it once apache#26400 merges. ## Are these changes tested? Yes, `window.slt` cases for `HAVING` (inline, named, undefined name) and `WHERE` (inline, named). The `HAVING` cases fail without the fix. ## Are there any user-facing changes? Queries that used a named window in `HAVING` now return an error instead of wrong results.
Which issue does this PR close?
Rationale for this change
OVER (w ROWS BETWEEN ...)silently dropped the named windoww. The planner only resolvedOVER w; for a name inside the parentheses it ignoredWindowSpec::window_name, so the window kept the frame but lostw'sPARTITION BYandORDER BYand produced wrong results without an error.What changes are included in this PR?
match_window_definitionsnow resolves a named window referenced insideOVER (...)the way the SQL standard and PostgreSQL do:PARTITION BYandORDER BY, and adds its own frame.ORDER BYonly when the referenced window has none.PARTITION BY, to override an existingORDER BY, or to copy a window that has a frame.A window definition can also reference an earlier definition, such as
WINDOW w2 AS (w1 ROWS ...). Only earlier definitions can be referenced, so references cannot form a cycle.What is the testing strategy for this PR?
New cases in
window.sltcover inheriting with an added frame, adding anORDER BY, a definition that references an earlier one,OVER (w), and each error. All of them fail without the fix.Are there any user-facing changes?
Queries that used
OVER (w ...)now return correct results. Queries that relied on the old behavior to overridew'sPARTITION BYorORDER BYnow get a planning error, as in PostgreSQL.