dolthub/dolt#10866: fix(sql-shell): parse input per character instead of per line#11202
Conversation
Replace the per-line semicolon suffix check in the interactive SQL shell with the existing per-character StreamScanner, fixing four shell bugs (dolthub#10860, dolthub#10861, dolthub#10862, dolthub#10865). The ishell accumulator decided "done?" by testing whether the trimmed line ended with ";". That test has no awareness of SQL quotes, comments, escapes, or the active DELIMITER, which causes the shell to stop too early (semicolon inside a quote or comment) or never stop (empty Enter triggers continuation mode). Also, a separate assumption that the accumulated buffer is always one statement caused multi-statement lines to fail when fed to a single-statement parser. This consumes the IsComplete callback added to the ishell fork via a go.mod replace directive (codeaucafe/ishell@061915e), pending the upstream ishell PR merge. - Add NewStreamScannerWithDelimiter to seed a fresh scanner with the shell's active delimiter; the shell scans one command at a time so the delimiter set by a prior DELIMITER command must be passed in rather than discovered in-band as batch mode does - Add endedAtEOF bool field on StreamScanner (reset each Scan call, set when input ends before the delimiter) and three accessors: InsideQuote, InsideBlockComment, EndedAtEOF - Add IsShellInputComplete(text, delimiter string) bool which drives the scanner over the accumulated buffer; returns false when input ends inside an open quote or block comment, or with a trailing unterminated statement; blank input returns true (fixes dolthub#10865) - Set IsComplete: IsShellInputComplete in UninterpretedConfig so the completeness decision is SQL-aware (fixes dolthub#10861, dolthub#10862, dolthub#10865) - Replace single sqlparser.Parse(wholeBuffer) call in the shell callback with a NewStreamScannerWithDelimiter split-and-execute loop; each statement in the buffer is parsed and executed individually, printing its own result set (fixes dolthub#10860); per-statement errors print and execution continues, matching MySQL CLI semantics - Add four PTY-driven bats tests via expect scripts, one per issue, that drive a real dolt sql session to verify each fixed behavior Refs: dolthub#10866
| github.com/cncf/xds/go v0.0.0-20251210132809-ee656c7534f5/go.mod h1:KdCmV+x/BuvyMxRnYBlmVaq4OLiKW6iRQfvC62cvdkI= | ||
| github.com/cockroachdb/apd/v3 v3.2.3 h1:4Zx+I3R35bFXMnltzmjP79i2cravE4jTRL6ps9Aux80= | ||
| github.com/cockroachdb/apd/v3 v3.2.3/go.mod h1:klXJcjp+FffLTHlhIG69tezTDvdP065naDsHzKhYSqc= | ||
| github.com/codeaucafe/ishell v0.0.0-20260607220657-061915e86568 h1:zAWHQAgb62ygwjQsDscI9btOt/lWEa5NbgMjKL2y1qM= |
There was a problem hiding this comment.
this should be dolthub/ishell
angelamayxie
left a comment
There was a problem hiding this comment.
please take a look at the failing tests
| gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect | ||
| ) | ||
|
|
||
| replace github.com/dolthub/ishell => github.com/codeaucafe/ishell v0.0.0-20260607220657-061915e86568 |
There was a problem hiding this comment.
remove this line. run go get github/com/dolthub/ishell@[commit] to update go.mod and go mod tidy to clean up go.sum
There was a problem hiding this comment.
I don't believe this works for forks/outside contributors like myself. I had to do this same thing when making changes in Doltgres that a Dolt Pr I made needed for it to work/be fixed. That's why I did it this way. I'll find those PRs later tonight to show u what I mean.
There was a problem hiding this comment.
forgot to add: but I'll also try that just to be sure again
There was a problem hiding this comment.
Okay, I think you're right this doesn't work because it's a forked repo, instead of a regular branch in the original repo.
…e/fix/10866-shell-per-character-input-parsing # Conflicts: # go/go.sum
cec63bf to
b9eacae
Compare
|
@angelamayxie thank you for reviewing. the following CI always fail for my outside contributor PRs:
I made a PR the same way in Doltgres so we can get this integration test passing: dolthub/doltgresql#2846 |
|
FYI still working on PR refactor. Made the changes, but reviewing and fixing some issues I found. |
Summary
The interactive
dolt sqlshell accumulated input one physical line at a time and decided "is this statement done?" by checking whether the trimmed line ended with;. That check has no awareness of SQL quotes, comments, or the activeDELIMITER, causing four bugs reported in #10860–#10862 and #10865. This PR replaces the blind suffix check with dolt's existing per-characterStreamScanner(already used by batch mode) and replaces the single-statementParsecall in the shell callback with a scanner-driven split-and-execute loop.The shell-side completeness check is wired in via a new opt-in
IsCompletecallback added to thedolthub/ishellfork (PR: dolthub/ishell#8).go/go.modcontains areplacedirective pointing at to PR's head commit:061915e.Issues fixed
select null; select null;on one line gavesyntax error at position 20 near 'select'. The accumulated buffer was fed tosqlparser.Parseonce; that parser accepts only a single statement.select ';was treated as complete because the line ends with;, even though that;is inside an open string literal.select /* ;andselect -- ;were treated as complete for the same reason; the;is inside an open block or line comment.->continuation prompt instead of re-issuing the primary>prompt.Changes
go/cmd/dolt/commands/sql_statement_scanner.goNewStreamScannerWithDelimiter(r io.Reader, delimiter string)constructor. The shell processes one command at a time, so the delimiter set by a priorDELIMITERcommand must be passed in rather than discovered in-band (as batch mode does when scanning a whole script).NewStreamScannerdelegates to this and is unchanged for existing callers.endedAtEOF boolfield onStreamScanner, reset at the top of eachScan()call and set when the input runs out before the active delimiter is matched.InsideQuote(),InsideBlockComment(),EndedAtEOF().IsShellInputComplete(text, delimiter string) bool. Runs the scanner over the accumulated buffer and returnsfalsewhen the input ends inside an open quote or block comment, or with a trailing unterminated statement. Blank/whitespace-only input returnstrueso an empty Enter does not trigger continuation mode (fixes Dolt shell expects more input after empty line whereas MySQL shell returns nothing #10865).go/cmd/dolt/commands/sql.goIsComplete: IsShellInputCompleteinUninterpretedConfig. The ishell library calls this after each physical line with the cumulative buffer and the current delimiter; returningtruestops accumulation. This is the single change that makes completeness detection SQL-aware (fixes Dolt shell does not ignore delimiters inside quotes #10861, Dolt shell does not ignore delimiters inside comments #10862, Dolt shell expects more input after empty line whereas MySQL shell returns nothing #10865).sqlparser.Parse(query)call in theUninterpretedcallback with aNewStreamScannerWithDelimitersplit-and-execute loop. Each statement in the buffer is parsed and executed individually so multiple;-separated statements on one line each run and print their own result set (fixes Dolt shell has issues parsing multiple statements in the same line #10860). Per-statement errors are printed and execution continues with the next statement, matching MySQL CLI semantics.Testing
Unit tests (
go/cmd/dolt/commands/sql_statement_scanner_test.go)TestIsShellInputComplete— 22 cases covering each issue branch (multi-statement, open quote, open block/line comment, empty input) plusDELIMITERcases.TestEndedAtEOFFlag— 3 sub-tests verifying theendedAtEOFflag is set on EOF-terminated input and cleared on delimiter-terminated input.Integration tests (
integration-tests/bats/sql-shell.bats)Four new bats tests, one per issue, each driving a real interactive
dolt sqlsession:sql-shell: multiple statements on one line (#10860)sql-shell: unclosed quote continues input (#10861)sql-shell: unclosed comment continues input (#10862)sql-shell: empty line stays at primary prompt (#10865)Merge Strategy
Fixes: #10860, #10861, #10862, #10865, #10866