Uh oh!
There was an error while loading. Please reload this page.
fix: nested window function - #15033
Conversation
chenkovsky
commented
Mar 5, 2025
sqllogictest complains stackoverflow. because I added a recursive visitor. but if I add stacksize, it works. so I think it's not real stackoverflow. Let me check how to refine it. |
chenkovsky
commented
Mar 6, 2025
@alamb I think I have to add a BFS visitor in sqlparse crate, how do you feel about it? |
alamb
commented
Mar 6, 2025
i think that would mean it will take at least another month to fix this issue - as we would need a new sql parser release and then integrate that into DataFusion If you can find another way that would likely be faster to get in |
I increased stack size in test |
chenkovsky
commented
Mar 8, 2025
i'm working on rewriting visit logic. but I found this pr apache/datafusion-sqlparser-rs#1522 . does it mean that this pr doesn't take effect on the test 🤔 |
chenkovsky
commented
Mar 8, 2025
by the way, If I change tokio to single thread, there's also no stack overflow. |
chenkovsky
commented
Mar 12, 2025
@alamb could you please review it agian. I have found the solution that doesn't need to touch stack size. |
| SUM(t1.v1) OVER w + 1 | ||
| FROM | ||
| generate_series(1, 10000) AS t1(v1) | ||
| WINDOW |
There was a problem hiding this comment.
I think we should at least have a .slt test that shows this query running and producing the same result as postgres, perhaps with a smaller number of series:
postgres=# SELECTt1.v1,
SUM(t1.v1) OVER w
FROM
generate_series(1, 5) AS t1(v1)
WINDOW
w AS (ORDER BYt1.v1 ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW);
v1 | sum
----+-----1 | 12 | 33 | 64 | 105 | 15
(5 rows)| NamedWindowExpr::WindowSpec(spec) => { | ||
| WindowType::WindowSpec(spec.clone()) | ||
| let mut err = None; | ||
| visit_expressions_mut(expr, |expr| { |
There was a problem hiding this comment.
I am sorry @chenkovsky and @2010YOUY01
I don't know what
SELECTt1.v1,
SUM(t1.v1) OVER w +1FROM
generate_series(1, 10) AS t1(v1)
WINDOW
w AS (ORDER BYt1.v1);
Is supposed to be computing (what does adding one to a window definition like w +1 represent?)
There was a problem hiding this comment.
DuckDB interprets it as (SUM(t1.v1) OVER w) + 1, and ... OVER (w + 1) is not valid
D SELECT
t1.v1,
SUM(t1.v1) OVER (w + 1)
FROM
generate_series(1, 10) AS t1(v1)
WINDOW
w AS (ORDER BY t1.v1);
Parser Error: syntax error at or near "+"
LINE 3: SUM(t1.v1) OVER (w + 1)
bb373ca to
ffa9124Comparechenkovsky
commented
Apr 4, 2025
could anyone help to review thir pr? |
alamb
commented
Apr 4, 2025
@jonahgao would you possibly have time to help review this PR? |
| named_windows | ||
| { | ||
| if let Some(WindowType::NamedWindow(ident)) = &f.over { | ||
| if ident.eq(window_ident) { |
There was a problem hiding this comment.
Unrelated to the current fix, we should compare them using normalized names to support
SELECTt1.v1,
SUM(t1.v1) OVER W +1FROM
generate_series(1, 5) AS t1(v1)
WINDOW
w AS (ORDER BYt1.v1);* fix: nested window function * prevent stackoverflow * Update select.rs * Update sqllogictests.rs * Update sql_api.rs * Update select.rs * Update sqllogictests.rs * update slt * update slt * clippy
Which issue does this PR close?
Rationale for this change
current implementation doesn't support nested window function in projection.
What changes are included in this PR?
use expr visitor to find nested window function.
Are these changes tested?
unit test
Are there any user-facing changes?
No