Skip to content

fix(sql): use AST-based statement type detection for isSelectQuery#113

Open
veksen wants to merge 1 commit intomainfrom
veksen/fix-select-query-detection
Open

fix(sql): use AST-based statement type detection for isSelectQuery#113
veksen wants to merge 1 commit intomainfrom
veksen/fix-select-query-detection

Conversation

@veksen
Copy link
Copy Markdown
Member

@veksen veksen commented Apr 13, 2026

Summary

  • Uses analysis.statementType from @query-doctor/core (AST-based) to determine isSelectQuery instead of the regex /select/i
  • The unanchored regex matched SELECT anywhere in the query, misclassifying UPDATE/INSERT/DELETE queries that contain SELECT subqueries (e.g. UPDATE ... FROM (SELECT ...))
  • Falls back to an anchored regex ^\s*select for skipped queries (>50KB) where analysis is not available
  • Adds 6 new tests covering DML with SELECT subqueries and analyze()-based detection

Dependencies

Requires @query-doctor/core >= 0.8.1 which adds statementType: StatementType to AnalysisResult. Companion PR: Query-Doctor/Site#2710.

Why the regex existed

CTE queries like WITH cte AS (...) SELECT ... start with WITH, not SELECT. The non-anchored regex was likely intended to catch those. The AST-based approach handles CTEs correctly because the parser identifies the outermost statement type (SelectStmt for WITH ... SELECT, UpdateStmt for WITH ... UPDATE).

Test plan

  • Existing isSelectQuery tests still pass (SELECT, INSERT, UPDATE, DELETE)
  • New tests verify DML with SELECT subqueries are correctly classified as non-SELECT
  • New analyze() integration tests verify AST-based detection for CTE, UPDATE+subquery, INSERT...SELECT, DELETE+EXISTS
  • Compat CI should pass once paired with the core PR

🤖 Generated with Claude Code

The previous regex `/select/i` (no anchor) matched SELECT anywhere in
the query, causing UPDATE/INSERT/DELETE queries with SELECT subqueries
to be misidentified as SELECT queries. This sent them into the
optimization pipeline where they'd hit EXPLAIN errors.

Now uses `analysis.statementType` from @query-doctor/core when
available (AST-based, handles CTEs correctly). Falls back to an
anchored regex `/^\s*select/i` for skipped queries (>50KB).

Requires @query-doctor/core >= 0.8.1 (adds statementType to
AnalysisResult).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Query Doctor Analysis

View full run details

83 queries analyzed

2 pre-existing issues

Using assumed statistics (10000000 rows/table). For better results, sync production stats.

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