Parser: fix exponential parse time on chains of IN (

[moshap-firebolt]

parse_in picks between a subquery and an expression list by speculatively parsing a query and rolling back. The list fallback then recurses back into parse_in over the same tail — parse_expr accepts a reserved word as an identifier — so each nesting level re-attempts the identical speculative parse.

Not working before:

"SELECT NOT IN(".repeat(20)   -- 1.3s
"SELECT NOT IN(".repeat(26)   -- 83s

Memoize the failed positions, as parse_table_factor already does for the FROM (((( shape it has the same structure as. A cached failure yields the same fallback as re-running the parse, so behaviour is unchanged.

Found by a fuzzer. Reproduces on upstream main too.

[moshap-firebolt]

Both findings were correct — thanks. The first was a real regression: committing on the opening keyword rejected IN (select), IN (values), IN (value, other) and friends, which all parsed before. My acceptance matrix had no keyword-as-identifier cases, so it missed them.

Fixed in f1fb8ca by narrowing the commit condition to SELECT/WITH and the keyword not being the whole list element. That also resolves the MERGE finding — VALUES, TABLE, MERGE, INSERT/UPDATE/DELETE now stay on the speculative path rather than being routed by an incomplete keyword list. All 11 keyword cases are back to parsing, the chain is still linear (160 levels, <10ms), and the dispatch test now pins them.

[moshap-firebolt]

The SELECT NOT IN(\n chain is linear after this; I checked depth 20/40 on GenericDialect.

Two issues below: a behaviour change on IN (select()) / IN (WITH()), and a residual exponential path on an extra (.

(The Bugbot note about missing MERGE in peek_query_body_start is stale — that helper is gone.)

[moshap-firebolt]

Behaviour change. SELECT/WITH followed by ( commits to parse_query, so these go from InList on firebolt/v0.62.0-patches to a parse error:

• SELECT 1 WHERE x IN (select()) — was InList of function select(), now Expected: an expression, found: )
• SELECT 1 WHERE x IN (WITH()) — was InList of function with(), now Expected: identifier, found: (

parse_query("SELECT()") fails, and committing drops the list fallback. The claim that no input changes verdict is false.

Don't commit on Token::LParen either. IN (SELECT(1)) then stays speculative and still becomes InSubquery (query-first, same as today). The fuzzer chain SELECT NOT IN(\nSELECT still commits — second token is NOT.

parse_in_subquery_vs_list_dispatch would not have caught this: it only checks that parsing succeeds, and it never includes IN (select()). Assert InList vs InSubquery.

[moshap-firebolt]

Residual exponential path. This arm is unchanged, so SELECT NOT IN((\n repeated is still 2^depth. A debug run of that shape at depth 8+ had not finished after 90s; the single-paren fuzzer input is fixed.

A dialect-fuzzer mutant that inserts one ( gets the original timeout back. Worth a follow-up (or extending the regression test to this shape with a timeout) if this is meant to close FB-3290 rather than just the minimized seed.

[moshap-firebolt]

Thanks — first finding confirmed and fixed, second I cannot reproduce.

1. IN (select()) / IN (WITH()) — real regression, fixed in d6ad549. Verified against firebolt/v0.62.0-patches: both parsed before my change and errored after. The lookahead now also declines to commit when the keyword is followed by (, so a bare word used as a function name stays on the speculative path alongside its use as an identifier. IN (SELECT (1)) still parses (it just speculates), the chain is still linear (160 levels, 8ms), and select() / WITH() / select(1), 2 are now pinned in parse_in_subquery_vs_list_dispatch.

2. Residual exponential on SELECT NOT IN(( — not reproducing. Measured both builds, GenericDialect, -runs=1:

depth	release	debug
24	0.001s	0.002s
40	0.001s	0.011s
80	0.008s	0.087s

Linear, and the same shape is equally fast on the unpatched parser (0.005s at 160), so it is not a regression from this change either. Your structural point is right that the ( arm still speculates — but because every other case now commits, the speculation is bounded to one attempt per ( rather than compounding, which is what these numbers show.

If you have the exact input, I will happily take it — a shape I have not found beats my sampling. Otherwise I would rather not add a regression test asserting a bound I cannot demonstrate is currently violated.

The stale-peek_query_body_start note is right, and IN (MERGE …) does still parse.

[moshap-firebolt]

Fair, and worth fixing precisely because this PR would be the first thing an agent reads next to that file. Trimmed in 4fe47aa: comment lines added to src/ go 24 → 9, against the 5–7 the file cites as typical; commit body 14 → 11 lines; PR description down to the bug, an example, and the fix. Test preambles and the per-group comments in the dispatch list are gone — the test names and the SQL carry it. Suite still green, clippy clean.

[moshap-firebolt]

Confirmed and fixed in cc88ea7 — and this one was worse than a parse error, so thank you for it.

IN (VALUES (1)), IN (VALUES (1), (2)) and IN (SELECT (1)) all still parsed, but silently became InList instead of InSubquery. My acceptance matrix only recorded parse success, so it showed no change at all while the AST was quietly wrong. Measured against firebolt/v0.62.0-patches:

                                    baseline      before this fix
IN (VALUES (1))                     InSubquery -> InList
IN (VALUES (1), (2))                InSubquery -> InList
IN (SELECT (1))                     InSubquery -> InList

Fixed by inverting the structure: the speculative maybe_parse(parse_query) path is now the default and is reached by everything, exactly as before. Only the unambiguous SELECT/WITH case commits, which is the single case the exponential needs. All 23 cases now match baseline on node type, not just on parsing, and the chain is still linear (160 levels, 7ms; ((, SELECT( and VALUES shapes flat too).

parse_in_subquery_vs_list_dispatch now asserts the node type per case. Verified it is a real gate: reintroducing the v3 behaviour makes it fail on IN (VALUES (1)). It also caught a wrong assumption of mine while I wrote it — select(1), 2 is the query SELECT (1), 2, so it is InSubquery on baseline too.

[moshap-firebolt]

Since three regressions here got past hand-written case lists, I stopped writing case lists and ran a differential test instead: same harness built against firebolt/v0.62.0-patches and against this branch, dumping the whole AST ({:?} of the statement vector) or the error text for every input, across all 15 dialects.

533 inputs — every IN (...) inner form I could think of (query openers as keywords, as identifiers and as function names; empty; parenthesized; nested IN inside a subquery; set ops; UNNEST) crossed with 9 statement contexts (WHERE, row-constructor IN, CASE WHEN, DELETE, UPDATE, nested subquery, two INs in one predicate), plus the four nesting shapes at depths 1–8. 7995 parses per build.

Result:

• accept/reject: identical on all 7995. Nothing that parsed now errors, nothing that errored now parses.
• all 6921 successful parses: byte-identical ASTs. No node-type or structural change anywhere, which is the class of bug the InSubquery -> InList finding belonged to.
• the only difference is error message text, on 12 malformed inputs, all ERR -> ERR. They are the IN (WITH c AS (SELECT 1)) family — a CTE with no body — plus IN (), IN (TABLE t), IN ((SELECT 1), (SELECT 2)) and IN (FROM t SELECT a) when nested inside a subquery.

That last one is worth a look rather than hiding in a footnote, since it is a real if minor change:

IN (WITH c AS (SELECT 1))
  before: Expected: ), found: c                                            (col 27)
  after:  Expected: SELECT, VALUES, or a subquery in the query body ...    (col 42)

Committing means the error now comes from the query parser at the point the body is missing, instead of from the list parser complaining about the CTE name. I think the new message is the better one — it names what is actually missing — but it is a change, so flagging it explicitly. Happy to keep the old text if error-message stability matters here.

Harness and corpus are throwaway, but I can attach them if useful.

[cursor]

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

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

^{Reviewed by Cursor Bugbot for commit cc88ea7. Configure here.}

[moshap-firebolt]

Confirmed — and this is the fourth regression from the same lookahead, so I have abandoned that approach entirely rather than patch it a fifth time.

Your finding reproduces: 12 forms x 3 contexts = 36 inputs went OK -> ERR, including IN (select.col), IN (select = 1), IN (select::int), IN (with.x), IN (select.*) and IN (select || 'a'). The problem is structural: a bare keyword used as an identifier can be followed by essentially any operator or postfix, so "what may follow SELECT/WITH" is not a closed set and every whitelist I write leaks.

88c8695 drops the lookahead and memoizes the failed positions instead — the same mechanism parse_table_factor already uses for FROM ((((, which has this exact structure and cause. That fixes the 2^N without any dispatch change, so behaviour is unchanged by construction: a cached failure produces the same fallback as re-running the parse.

Evidence, differential against firebolt/v0.62.0-patches over 644 inputs x 15 dialects = 9660 parses:

• byte-identical output, including every error message. Not "same accept/reject", not "same AST" — the entire {:?} dump and every error string match exactly. The previous attempt changed error text on 12 malformed inputs; that difference is now gone too.
• chain is linear: 160 levels in 7ms, versus 21s at 24 levels and 83s at 26 on baseline.
• 28 candidate nesting shapes swept for any residual 2^N: all linear. I checked that sweep is sensitive enough to catch the original bug (baseline: 0.006s -> 0.09s -> 1.3s at depths 10/16/20).

Both gates verified by breaking them: disabling the cache makes parse_in_chain_no_exponential_blowup time out, and the dispatch test fails if node types shift.

One correction to my earlier numbers: the 21s figure came from the cargo-fuzz binary (coverage + sanitizers) at depth 20. A plain release build is 1.3s at depth 20 and 21s at depth 24. The PR text now uses the plain-release figures.

[moshap-firebolt]

Re-reviewed this against AGENTS.md now that #11 has merged. Three things were out of line; all fixed in 9d03b52.

1. Unnecessary refactor. I had extracted parse_in_list, but after the rewrite the list arm is used exactly once, so the helper only existed to make the old two-branch version readable. That is a refactor that is not required, which General Coding Guidelines #2 already prohibited before #11 landed. Inlined; the src diff drops from 34 lines to 22.

2. Test table was near-duplicate noise (rule 4). It had 19 cases, 9 of which pinned keyword-vs-identifier behaviour of a lookahead this version no longer contains — the diff adds zero Keyword:: references. Those cases were testing code that is not here. Trimmed to 9: both node types, the ((...)) ambiguity, SELECT (1), and one representative each of keyword-as-identifier, keyword-as-function and keyword-in-compound.

3. Comment budget (rule 1). 8 lines, now 5 — the block comment was restating the commit message.

Commit body is 8 lines, PR description is the bug, an example and the fix. Re-verified after the trim: still byte-identical to firebolt/v0.62.0-patches across 9660 parses, chain still linear at 160 levels, suite green, clippy clean.

Also added the shape to tensile as a feature so it is probed continuously rather than only living in a fuzz seed: themosha/tensilelib#6. Worth reading the caveat there — it is deliberately invalid SQL, which that repo normally forbids, because balanced variants parse on the first attempt and cannot reproduce this class of bug at all.