fix(explain): share one explainable-statement check across all six strategies - #274
Merged
Conversation
…rategies Every strategy carried its own `/^\s*SELECT\b/i`, and every one of them refused two statements its engine explains perfectly well: a CTE, and a SELECT behind a comment. The Explain button was simply absent for both. #271 fixed it for Druid only; this extends it to the other five and moves the check into one place, `explain/select-prefix.ts`. The CTE case was also an internal disagreement: the shared `analyzeQuery` already classifies `WITH ... SELECT` as a SELECT and injects a LIMIT into one, so six strategies declining it contradicted the rest of the pipeline. Verified per dialect rather than assumed - each statement below was wrapped by the REAL strategy, executed against the REAL engine, and rendered: postgres 18 CTE, line comment, block comment all explain mysql 9 CTE, line comment, block comment all explain sqlite CTE, line comment, block comment all explain clickhouse 26.7 CTE, line comment, block comment all explain couchbase 8.0.2 WITH binding, line, block comment all explain druid 37 CTE, line comment, block comment all explain Couchbase needed its own spelling: SQL++ `WITH alias AS (<expression>)` binds a value rather than a subquery. PostgreSQL is the one dialect that could not simply take the shared answer, and finding out why is the reason this was verified per engine. Its strategy emits `EXPLAIN (ANALYZE, ...)`, which EXECUTES the statement, and a data-modifying CTE is a write wearing a WITH: EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON) WITH t AS (INSERT INTO probe(id) VALUES (42) RETURNING id) SELECT * FROM t -> 0 rows in the table before, 1 row (42) after. Explaining performed the insert. So accepting WITH there without a screen would have turned the Explain button into a write. `postgres-json.ts` pairs the classification with `hasDataModifyingStatement()` and applies it ONLY to the `with` case: PostgreSQL refuses a data-modifying CTE anywhere but the top level ("WITH clause containing a data-modifying statement must be at the top level", verified), so one cannot hide behind a leading SELECT, and screening SELECTs too would strip the button off `SELECT 'insert'`, which explains fine today. The screen errs toward refusing - a read-only CTE that merely mentions a keyword loses its button rather than risking a write. `classifySelectPrefix` returns `"select" | "with" | null` rather than a boolean precisely so that asymmetry is expressible. The regex keeps the shape arrived at in #271, where all three of its alternatives had to be made unambiguous independently - no leading `\s*`, a line comment anchored to newline-or-end, and a tempered block-comment body. Its bounded-time guard moves here with it, since the property belongs to the regex rather than to Druid; Druid keeps a lighter test proving it still routes through. Six gates green plus coverage at 100% (28534 lines), and all nine explain files at 100% individually.
Review follow-ups on #274, all verified before being accepted. `select-prefix.test.ts` already covered `hasDataModifyingStatement` on a read-only CTE that merely mentions a keyword, but nothing pinned what `postgresJsonStrategy.buildSql` does with it. That is where the documented "errs toward refusing" behaviour actually matters, so it now has a test: WITH t AS (SELECT 'insert' AS x) SELECT * FROM t -> null Paired with the LIMIT of that over-reach, which is the more interesting half and was untested: the word boundary is what keeps the screen from swallowing every CTE that touches an `updated_at` column, so WITH t AS (SELECT updated_at FROM u) SELECT * FROM t still explains. Writing that pair is what caught a wrong assertion in the first draft - I had expected `updated_at` to be refused, and it is not, correctly. Also records two facts about DATA_MODIFYING's membership that were previously assumed: - MERGE is a REAL carrier, not a defensive guess. Live on PostgreSQL 18, `EXPLAIN (ANALYZE, FORMAT JSON) WITH t AS (MERGE INTO probe ... RETURNING id) SELECT * FROM t` really inserted the row. - TRUNCATE is deliberately absent. It cannot ride inside a WITH at all - `WITH t AS (TRUNCATE probe) SELECT 1` is a SYNTAX error - and a statement leading with it never reaches the screen because the prefix classification already refuses anything but SELECT or WITH. Tested for TRUNCATE, DROP and CREATE together. The third review point - that `analyzeQuery` still carries its own regexes - turned out to hide a live defect rather than only a drift risk: a leading comment makes it classify a SELECT as OTHER, so `prepareQuery` injects no LIMIT and the query runs unbounded. Filed as #275 with the reproduction; it feeds the query path of all ten providers and belongs in its own change rather than in an explain PR.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Follow-up to #271, which fixed this for Druid only.
All six explain strategies carried their own copy of
/^\s*SELECT\b/i, and every one of them refusedtwo statements its engine explains perfectly well — a CTE, and a SELECT behind a comment. The
Explain button was simply absent for both. The check now lives in one place,
src/lib/explain/select-prefix.ts.The CTE case was also an internal disagreement: the shared
analyzeQuery(
db/utils/query-limiter.ts) already classifiesWITH … SELECTas a SELECT and injects aLIMITinto one, so six strategies declining itcontradicted the rest of the pipeline.
Verified per dialect, not assumed
Each statement below was wrapped by the real strategy, executed against the real engine, and
rendered through
extractPlan→toRenderModel:WITHbinding, line comment, block commentCouchbase needed its own spelling — SQL++
WITH alias AS (<expression>)binds a value, not asubquery. That is the kind of thing a shared regex would have papered over.
PostgreSQL could not take the shared answer, and that is why this was verified per engine
Its strategy emits
EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON), which executes the statement — and adata-modifying CTE is a write wearing a
WITH:Explaining performed the insert. So accepting
WITHthere without a screen would have turned theExplain button into a write — a regression introduced by a change whose whole purpose was consistency.
postgres-json.tspairs the shared classification withhasDataModifyingStatement()and applies thatscreen only to the
"with"case. Two reasons it is scoped that way, both verified:data-modifying statement must be at the top level" — so one cannot hide behind a leading
SELECT.SELECTtoo would strip the button off anything that merely mentions a keyword.SELECT 'insert' AS wordandSELECT updated_at FROM texplain fine today and still do.The screen errs toward refusing: a read-only CTE that merely mentions
insertloses its Explainbutton rather than risking a write. That is the only direction this can safely err in, and it is
documented as such.
classifySelectPrefix()returns"select" | "with" | nullrather than a boolean precisely so thatasymmetry is expressible by a caller.
The regex shape is carried over deliberately
#271 arrived at this pattern the hard way — all three alternatives sit inside a
*quantifier, so eachhad to be made unambiguous independently:
\s*beside a\salternative[\s\S]*?\*\/spanning two comments--[^\n]*with no(?:\n|$)tailThe third was found by CodeQL after the first two were fixed, and it is the one worth remembering: it
needs three orders of magnitude less input, and it slipped past a guard that exercised
-- a\nrepetitions, because the newline is exactly what makes that branch unambiguous.
The bounded-time guard moves into
select-prefix.test.tswith the regex — the property belongs to thepattern, not to Druid — and now covers bare-dash runs explicitly. Druid keeps a lighter test proving it
still routes through the shared check.
Verification
All 8 files under
src/lib/explain/that carry executable lines are at 100% individually —clickhouse-json,couchbase-json,druid-native,index,mysql-json,postgres-json,select-prefix,sqlite-queryplan. The ninth,types.ts, is types only and so never appears in thelcov at all.
(An earlier revision of this description said "nine files under
src/lib/explain/". That was wrong:the grep behind it matched
/explain/and so also caughtsrc/app/api/ai/explain/route.ts, which isan API route rather than a strategy. Corrected here — thanks to the reviewer who asked for the
coverage report to be confirmed rather than taken on trust.)
docs/ADDING_A_PROVIDER.mdgains both rules: useclassifySelectPrefix()rather than a fresh regex,and ask whether your engine's EXPLAIN executes before widening what it accepts.
Review follow-ups (a8a224e)
select-prefix.test.tscoveredhasDataModifyingStatementon a read-only CTE mentioning a keyword, but nothing pinned whatbuildSqldoes with it. It now does — together with the limit of that over-reach, which was theuntested half: the word boundary is what stops the screen swallowing every CTE that touches an
updated_atcolumn, so that case still explains. Writing the pair caught a wrong assertion in myfirst draft, where I had expected
updated_atto be refused.DATA_MODIFYINGmembership is now recorded rather than assumed.MERGEis a real carrier —live,
EXPLAIN (ANALYZE) WITH t AS (MERGE INTO probe … RETURNING id) SELECT * FROM tinserted therow.
TRUNCATEis deliberately absent: it cannot ride inside aWITHat all(
WITH t AS (TRUNCATE probe) SELECT 1is a syntax error), and a statement leading with it isalready refused by the prefix classification. Tested for
TRUNCATE,DROPandCREATE.analyzeQuerynote turned out to hide a live defect, not just drift risk. A leading commentmakes it classify a
SELECTasOTHER, soprepareQueryinjects noLIMITand the query runsunbounded — the same root cause as this PR, with a worse outcome. Filed as A leading SQL comment defeats the query limiter, so a commented SELECT runs unbounded #275 with the
reproduction. Deliberately not fixed here:
analyzeQueryfeeds the query path of all ten providersplus pagination, so it needs its own change and its own tests.