Skip to content

dolthub/dolt#10866: fix(sql-shell): parse input per character instead of per line#11202

Open
codeaucafe wants to merge 2 commits into
dolthub:mainfrom
codeaucafe:codeaucafe/fix/10866-shell-per-character-input-parsing
Open

dolthub/dolt#10866: fix(sql-shell): parse input per character instead of per line#11202
codeaucafe wants to merge 2 commits into
dolthub:mainfrom
codeaucafe:codeaucafe/fix/10866-shell-per-character-input-parsing

Conversation

@codeaucafe

@codeaucafe codeaucafe commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Summary

The interactive dolt sql shell 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 active DELIMITER, causing four bugs reported in #10860#10862 and #10865. This PR replaces the blind suffix check with dolt's existing per-character StreamScanner (already used by batch mode) and replaces the single-statement Parse call 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 IsComplete callback added to the dolthub/ishell fork (PR: dolthub/ishell#8). go/go.mod contains a replace directive pointing at to PR's head commit: 061915e.

Issues fixed

  • #10860select null; select null; on one line gave syntax error at position 20 near 'select'. The accumulated buffer was fed to sqlparser.Parse once; that parser accepts only a single statement.
  • #10861select '; was treated as complete because the line ends with ;, even though that ; is inside an open string literal.
  • #10862select /* ; and select -- ; were treated as complete for the same reason; the ; is inside an open block or line comment.
  • #10865 — pressing Enter on an empty line dropped the shell into the -> continuation prompt instead of re-issuing the primary > prompt.

Changes

go/cmd/dolt/commands/sql_statement_scanner.go

  • Add NewStreamScannerWithDelimiter(r io.Reader, delimiter string) constructor. The shell processes 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 when scanning a whole script). NewStreamScanner delegates to this and is unchanged for existing callers.
  • Add endedAtEOF bool field on StreamScanner, reset at the top of each Scan() call and set when the input runs out before the active delimiter is matched.
  • Add three accessors exposing parse state to callers: InsideQuote(), InsideBlockComment(), EndedAtEOF().
  • Add IsShellInputComplete(text, delimiter string) bool. Runs the scanner over the accumulated buffer and returns false when the input ends inside an open quote or block comment, or with a trailing unterminated statement. Blank/whitespace-only input returns true so 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.go

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) plus DELIMITER cases.
  • TestEndedAtEOFFlag — 3 sub-tests verifying the endedAtEOF flag 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 sql session:

  • 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

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
Comment thread go/go.sum Outdated
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=

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be dolthub/ishell

@angelamayxie angelamayxie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please take a look at the failing tests

Comment thread go/go.mod
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
)

replace github.com/dolthub/ishell => github.com/codeaucafe/ishell v0.0.0-20260607220657-061915e86568

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove this line. run go get github/com/dolthub/ishell@[commit] to update go.mod and go mod tidy to clean up go.sum

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

forgot to add: but I'll also try that just to be sure again

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
@codeaucafe codeaucafe force-pushed the codeaucafe/fix/10866-shell-per-character-input-parsing branch from cec63bf to b9eacae Compare June 14, 2026 02:27
@codeaucafe

codeaucafe commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

@angelamayxie thank you for reviewing. the following CI always fail for my outside contributor PRs:

  • Benchmark Mini Sysbench...
  • Benchmark SQL Correctness...
  • Check for correctness_approved label...
  • Check for performance_approved label...
  • Check formatting, Commuters....

Tests Bats Unix Remote/Bats tests is failing at the Install Maven step. For some reason this CI gate has been failing at this step for me for my last 3-4 contributions this year. I'm not entirely sure and never got a reason when I asked and looked into it.

Test Integration with DoltgreSQL/ test-integration... is failing due to it requiring my change to ishell to pass. I had this same issue with before with a change I made to dolt that required me to make use the replace directive pointing in DoltgreSQL in order for this DoltgreSQL integration test suite to pass. This is why I state previously that this replace directive is required for forked contributions like the ones I have to make as an outside contributor. @nicktobey helped me on that change. These are the two PRs for that for reference/example:

I made a PR the same way in Doltgres so we can get this integration test passing: dolthub/doltgresql#2846

Comment thread go/cmd/dolt/commands/sql_statement_scanner.go
@codeaucafe

codeaucafe commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

FYI still working on PR refactor. Made the changes, but reviewing and fixing some issues I found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants