Repository navigation
Conversation
ac9702d to
b7cbd09
Compare
|
@fmcmac could you take a look at the ci failures when you have the time? |
|
Is there a real issue / usecase (like a sql query you parsed) that this is fixing? |
|
Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look |
LucaCappelletti94
added
the
waiting on contributor
span() recurses once per AST node with no stack protection. A query can parse, then overflow the stack in span(). A generated 10,000-term OR chain does this on an 8 MB thread. Annotate span() with recursive::recursive, as the parser does. This covers Query, SetExpr, Statement, Expr, SelectItem, TableFactor, Select and ObjectNamePart. The attribute applies only with the recursive-protection feature. Generated-by: Claude Code
4aa3bc9 to
2fd0d26
Compare
fmcmac
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2400 +/- ##
==========================================
+ Coverage 82.54% 83.26% +0.72%
==========================================
Files 42 42
Lines 35553 35146 -407
Branches 35553 35146 -407
==========================================
- Hits 29346 29264 -82
+ Misses 2672 2360 -312
+ Partials 3535 3522 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Rebased onto main as 1 commit. A real use case is an OR chain, such as SELECT * FROM t WHERE id = 0 OR id = 1 OR ... OR id = 9999. It parses and prints, then span() aborts the process with a stack overflow. DataFusion's planner calls span() while planning each query. |
|
run benchmark sql_planner |
| expr = Expr::Nested(Box::new(expr)); | ||
| } | ||
| let _ = expr.span(); | ||
| // Dropping the chain recurses too; leak it so the test measures only the span walk. |
There was a problem hiding this comment.
If you can't drop the expression without overflowing the stack, I don't think allowing "spanned" to work is of much value
If we really want to support OR chains with 200,000 items, I think we need to use a non tree representation (there is an open ticket for this)
There was a problem hiding this comment.
Or maybe I am missing something 🤔
What do you think @LucaCappelletti94 ?
There was a problem hiding this comment.
Dropping a deep OR chain recurses too, but its frames are far smaller than those of span(). On a 2 MiB thread in release, span() on main overflows at about 2,400 OR terms, while dropping the same tree only overflows at about 32,000. In debug the figures are 313 against about 21,000. Of course all of these inputs are only adversarial stuff.
I agree with you that a different representation is the long term solution, and I think it is also worth adding that most of the spans current implementations are different flavours of wrong, so a refactoring regarding them will need to take place at some point in the future.
Dialects: none
Spanned::spanrecurses once per AST node without stack protection. A query can parse, then abort whenspan()walks it:On main this parses and prints, then
span()overflows an 8 MB stack. DataFusion's SQL planner callsspan()while planning each query.This adds
recursive::recursivetospan(), as the parser already does. It coversQuery,SetExpr,Statement,Expr,SelectItem,TableFactor,SelectandObjectNamePart. It applies only with the existingrecursive-protectionfeature. With it, 100,000 terms complete.2 tests walk 200,000-deep ASTs on a 512 KiB thread. Each overflows without the change. Adds 1 fuzz seed.
I wrote this PR with Claude Code, and I have read and reviewed every line.