fix(editor): stop treating a trailing comment as a second SQL statement - #1900
Merged
Conversation
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.
Fixes #1895.
Root cause
SQLStatementScanner.allStatementskeeps every segment that is non-empty after a whitespace trim, and comment text trims to a non-empty string. The scanner tracks comments to find the right split points but never asks whether a segment contains an executable token. That misfires in three places, not just the trailing one the issue named:SELECT 1; -- notesplits into two statements, the run takes the multi-statement path, and the comment is sent to the server, producing the spurious "Statement 2/2 failed" error.SELECT 1; /* x */; SELECT 2produces three statements.QueryClassifier.isMultiStatementcounts the same way, so the execution gate and the AI chat and MCP tools rejectedSELECT 1; -- noteas "multi-statement" before it even ran.Context: TablePlus fixed this exact bug in 2019 (TablePlus#485), DBeaver in 24.0.4 (dbeaver#26416), DataGrip never had it. ClickHouse is the one server that hard-errors on comment-only text (code 62), and its own
clickhouse-clientdrops trailing comments client-side.Fix
The
SQLFileParserapproach (hasStatementContent) transplanted into the scanner's existing state machine: one boolean tracked in the same character loop, passed as a third closure parameter.allStatementsandallStatementsPreservingSemicolonsskip segments without content; the cursor APIs ignore the flag, so autocomplete still sees raw comment segments and keeps suppressing suggestions inside comments./*!versioned comments count as content in every dialect, so MySQL dump statements like/*!40101 SET ... */;still execute.elseof the semicolon check, so a boundary semicolon never marks the segment it closes.Running a comment-only buffer, selection, or cursor-in-comment statement is now a no-op through the pre-existing
guard !statements.isEmptychecks at every call site, matching DataGrip and DBeaver. No production file changes besides the scanner.CI enablement (second commit)
The new cursor-API regression tests live in
SQLStatementScannerLocatedTests, which was on the CI quarantine list since the test gate landed (#1481). It was benched because of one broken precondition:largeInputgenerates 9,889 characters and asserts> 10_000, failing since March while quarantine hid it. The second commit bumps the generated input to 300 statements (about 14,800 characters) and removes the suite from the quarantine list. All 17 tests in the suite pass.Testing
;;still dropped, semicolon-preserving variant).QueryClassifier.isMultiStatementsuite and anExecutionGateTestscase proving the gate no longer denies trailing-comment SQL.xcodebuild test;swiftlint lint --strictreports nothing new on changed files.Follow-ups deliberately not in this PR: the scanner does not recognize
#or//line comments (a trailing# noteon ClickHouse can still error, a pre-existing splitting gap), andMCPConnectionBridgepasses a trailing comment through to single-exec drivers.