Avoid quadratic accumulation when parsing long SQL expression lists - #963
Avoid quadratic accumulation when parsing long SQL expression lists#963arc-oai wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe parser now accumulates expressions with linked lists and materializes them when building AST children. Clause and set-operation children use lazy getters. The large-query memory test is enabled. ChangesParser expression handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nene
left a comment
nene
left a comment
There was a problem hiding this comment.
Thanks for the PR. The fix seems to work, but the code is somewhat cryptic and seems to contain various extra stuff that's not really part of the core optimization proposed in this PR.
| interface PendingExpressions { | ||
| expressions: ExpressionList | null; | ||
| clauses: AstNode[]; | ||
| } |
There was a problem hiding this comment.
The name PendingExpressions makes no sense to me. It's the return value of the expressions_or_clauses rule.
| const materializeExpressions = ({ expressions, clauses }: PendingExpressions): AstNode[] => | ||
| materializeExpressionList(expressions).concat(clauses); |
There was a problem hiding this comment.
The name materializeExpressions is similarly an odd one. It doesn't align with the input data type name, nor does it align with the name of the rule expressions_or_clauses, together with which it is used.
| get children(): AstNode[] { | ||
| return [exp, ...materializeExpressionList(expressions)]; |
There was a problem hiding this comment.
What's the purpose of this wrapping of children property into a getter function?
It looks like some an additional performance optimization. But not sure it's really adding anything. At least when I removed these, the code still seemed to perform similarly.
If this is a separate unrelated optimization, then please make a separate PR with it. If it's tightly related to the current optimization and really needed for it, please provide some explanation.
| expression_list -> null {% () => null %} | ||
| expression_list -> expression_list free_form_sql {% |
There was a problem hiding this comment.
This definitely needs some documentation, as to why we're doing it this way. Otherwise somebody reading this code in the future will just refactor this into expression_list:*
| expressions_or_clauses -> expression_list clause:* {% | ||
| ([expressions, clauses]) => ({ expressions, clauses }) | ||
| %} |
There was a problem hiding this comment.
This also seems to be an additional optimization. Is this really needed? It doesn't seem to have much of an effect on the overall performance.
Again, I would prefer leaving this out and just doing concentrating on the core optimization proposed in this PR.
| } | ||
|
|
||
| interface ExpressionList { | ||
| previous: ExpressionList | null; |
There was a problem hiding this comment.
Note: Generally in SQL Formatter codebase we avoid the use of null, instead preferring undefined.
Like in here, better to use:
| previous: ExpressionList | null; | |
| previous?: ExpressionList; |
2 participants
Summary
INlists, longORchains, and wideSELECTlists.Why
Nearley expands
free_form_sql:*into an append postprocessor equivalent toitems.concat([item]). Repeatedly copying every prefix produces quadratic allocation. For a representative 1,000-valueINquery, these generated repetition postprocessors copied 1,997,014 prefix entries before this change and 10 afterward. Final AST construction still performs the necessary linear work: 3,004 list entries are materialized and another 3,000 entries are copied into completed arrays. Mutating the shared array with.push()is unsafe because alternative parses share prefixes.free_form_sqlremains the existing rule for one SQL element. The newexpression_listrecognizes exactly the same zero-or-more sequence asfree_form_sql:*:Each append instead creates an immutable
{ previous, value }node. Consequently,expressions_or_clauses -> expression_list clause:*preserves the original grammar: free-form elements followed by structured clauses. Clause materialization is deferred because eagerly converting every intermediate parser candidate would reintroduce quadratic copying.Benchmarks
Apple M4 Max, Node.js 24.12.0. Values show median formatting time and whole-process peak RSS. Stress processes used a 384 MB V8 old-space limit.
INININORchainSELECTValidation
pnpm run grammarpnpm run ts:checkpnpm run pretty:checkpnpm run lintpnpm run buildpnpm exec jest --runInBand --silent --coverage=false: 27 suites, 5,841 tests, and 63 snapshots passed.Closes #840.