Skip to content

Parser: fix exponential parse time on table-factor paren chains#2

Merged
moshap-firebolt merged 1 commit into
firebolt/v0.62.0-pathologicalfrom
moshap/fix-parse-table-factor-paren-blowup
Jun 9, 2026
Merged

Parser: fix exponential parse time on table-factor paren chains#2
moshap-firebolt merged 1 commit into
firebolt/v0.62.0-pathologicalfrom
moshap/fix-parse-table-factor-paren-blowup

Conversation

@moshap-firebolt

@moshap-firebolt moshap-firebolt commented Jun 9, 2026

Copy link
Copy Markdown

parse_table_factor speculatively parses (...) as a derived table; on
failure it rewinds and falls through to parse_table_and_joins, which
recurses back into parse_table_factor and re-attempts the same
speculative parse at every deeper paren. Both arms walk the remaining
chain, so on inputs like SELECT 1 FROM ((((... work doubles per level.
Caching the position at which the speculative arm failed short-circuits
the second descent.

Measured on GenericDialect with with_recursion_limit(256), release
build:

N Before After
10 20 ms 57 us
20 820 ms 119 us
25 2.8 s 281 us
30 7.9 s 345 us

Cherry-picked upstream as apache#2372 (against apache:main). Merging here so PackDB can pick the fix up while upstream review lands.


Note

Medium Risk
Touches core SQL FROM/join parsing with speculative-parse behavior; change is narrow (failure memoization only) but affects a hot, recursive path.

Overview
Fixes exponential parse time on malformed FROM inputs with long unclosed parenthesis chains (e.g. SELECT 1 FROM ((((...), where parse_table_factor repeatedly tried the derived-table path and the nested-join fallback on the same token positions.

Adds a failed_derived_table_factor_positions cache on Parser (cleared when tokens reset). Before calling parse_derived_table_factor, the parser skips positions already known to fail and records new failures so the nested-join path does not re-run the same speculative work.

Includes a timeout regression test and a Criterion benchmark (parse_table_factor_paren_chain) aligned with existing prefix/compound-chain perf benches.

Reviewed by Cursor Bugbot for commit dd977d0. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 5005c3f. Configure here.

Comment thread src/parser/mod.rs
@moshap-firebolt moshap-firebolt force-pushed the moshap/fix-parse-table-factor-paren-blowup branch from 5005c3f to 313b536 Compare June 9, 2026 19:31
`parse_table_factor` speculatively parses `(...)` as a derived table; on
failure it rewinds and falls through to `parse_table_and_joins`, which
recurses back into `parse_table_factor` and re-attempts the same
speculative parse at every deeper paren. Both arms walk the remaining
chain, so on inputs like `SELECT 1 FROM ((((...` work doubles per level.
Caching the position at which the speculative arm failed short-circuits
the second descent.

Measured on `GenericDialect` with `with_recursion_limit(256)`, release
build:

| N  | Before  | After  |
|----|---------|--------|
| 10 |  20 ms  | 57 us  |
| 20 | 820 ms  | 119 us |
| 25 |  2.8 s  | 281 us |
| 30 |  7.9 s  | 345 us |
@moshap-firebolt moshap-firebolt force-pushed the moshap/fix-parse-table-factor-paren-blowup branch from 313b536 to dd977d0 Compare June 9, 2026 19:35
@moshap-firebolt moshap-firebolt merged commit 2c43850 into firebolt/v0.62.0-pathological Jun 9, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant