Skip to content

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

Open
codeaucafe wants to merge 4 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 4 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 Outdated
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 Outdated
@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.

Complete the dolt side of moving the SQL statement scanner into the
ishell library so interactive shell input is read and scanned in a
single streaming pass. Previously dolt re-scanned each accumulated
line and then scanned the assembled command again; ishell now scans
once and delivers the parsed statements on the Context.

Delete the local StreamScanner and consume ishell's instead. The
interactive Uninterpreted handler runs the pre-scanned c.Statements
directly rather than re-splitting the query. The \edit transform
produces new SQL that ishell never saw, so that path still scans its
own editor output. Batch mode, debug, and filter-branch keep scanning
directly and are repointed at the relocated scanner.

- Remove sql_statement_scanner.go and its test; the scanner now lives
in github.com/dolthub/ishell
- Consume c.Statements in the shell.Uninterpreted handler; keep a
per-statement parse-and-execute loop for multi-statement input
- Re-scan only the \edit output via ishell.NewStreamScannerWithDelimiter
- Repoint execBatchMode, debug.go, and filter-branch.go to
ishell.NewStreamScanner and the exported StatementStartLine()
- Bump the dolthub/ishell dependency (replace) to the fork branch
hosting the relocated scanner and single-pass shell read path
- Add PTY bats tests: a large multi-line statement executes correctly,
and Ctrl-D at the continuation prompt discards partial input
- Tighten the unclosed-quote test to assert on the rendered result
cell rather than the echoed input line

Refs: dolthub#10866
…e/fix/10866-shell-per-character-input-parsing
codeaucafe added a commit to codeaucafe/doltgresql that referenced this pull request Jul 28, 2026
@codeaucafe

Copy link
Copy Markdown
Contributor Author

@angelamayxie I believe this is ready for another review. Most of the new changes were done in ishell like we discussed:

ishell (the meat) — PR #8

  • Relocated SQL StreamScanner into ishell so input can be scanned where the shell reads it — avoids a circular import.
  • Rewrote uninterpreted read path for single streaming pass: added uninterpretedReader (adapts line-at-a-time readline into an io.Reader) and readUninterpretedCommand, replacing the old per-line readMultiLinesFunc predicate. Resolved the O(n²) re-scan and the double-scan issues. I wrote a couple unit tests and benchmark tests to verify this, but I wasn't sure if it was overkill to include it or not. If you think its worth I'll make that commit and push it up
  • Added Context.Statements to give pre-scanned statements to consumers.
  • Fixed Ctrl-D at continuation prompt to discard the partial statement.
  • Cleanups: removed a no-op Delimiter() line, reordered a deferred-error check, trimmed stored statement text.
  • (Uncommitted local: the scaling test + benchmark.)

dolt — PR #11202

doltgresql — PR #2846

  • go.mod replace bump to the same ishell commit (950b93b)

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

Projects

None yet

3 participants