Repository navigation
Add opt-in IsComplete callback to UninterpretedConfig - #11
Open
codeaucafe wants to merge 7 commits into
Open
codeaucafe wants to merge 7 commits into
codeaucafe wants to merge 7 commits into
Conversation
…rminations Add optional IsComplete func(string) bool field to UninterpretedConfig. When non-nil, readUninterpreted dispatches accumulation through a new sibling readMultiLinesAccumFunc that exposes the cumulative buffer to the predicate, enabling consumers (such as the dolt SQL shell) to implement quote-, comment-, and escape-aware termination instead of the legacy per-line suffix check. The legacy line-suffix path is preserved for any consumer that leaves IsComplete nil, so existing consumers see no behavior change. Refs: dolthub/dolt#10866
…nterpreted Change UninterpretedConfig.IsComplete to func(accumulated, delimiter string) bool so the callback receives the delimiter currently in effect without the consumer having to capture the shell. Collapse the duplicated terminator logic in readUninterpreted into a single predicate plus a lineIsComplete helper, and make readMultiLinesFunc delegate to readMultiLinesAccumFunc to remove the duplicated accumulation loop. Refs: dolthub/dolt#10866
The IsComplete callback and supporting lineIsComplete helper were originally added to allow the accumulation loop to be tested w/ a standalone function (i.e., readMultiLinesAccumFromReader). Decided during PR review that this testing approach is unnecessary since the behavior is covered by Dolt BATS tests. Remove UninterpretedConfig.IsComplete, Shell.isComplete, and lineIsComplete. The SQL branch predicate in readUninterpreted now inlines the special-terminator, backslash-command, and line-terminator checks directly. Delete the standalone accumulation loop unit tests. Fix ReadMultiLinesFunc in actions.go to adapt to the two-argument readMultiLinesFunc predicate. Refs: dolthub/dolt#10866
Replace per-line readMultiLinesFunc predicate loop with a single streaming scan of uninterpreted-shell input. Before, each new line re-evaluated a completeness predicate over the whole accumulated buffer, and the consumer re-scanned the assembled command, an O(n^2) re-scan plus a redundant second scan. Relocate the SQL statement scanner into ishell as StreamScanner and add a line-at-a-time to io.Reader adapter so the scanner consumes terminal input directly, blocking for continuation lines only while mid-statement, and stopping at a special terminator. Parsed statements are stashed on the shell and delivered to handlers via the new Context .Statements field, so consumers execute them without scanning again. - Add StreamScanner: per-byte scan of quotes, comments, escapes, and multi-byte DELIMITER; relocated from dolt - Add uninterpretedReader: adapts shell's line reader into an io.Reader, strips \g and \G terminators from the scan feed while preserving them in Raw, and switches to the continuation prompt for pulled lines - Add readUninterpretedCommand: resolve line-level cases (empty, DELIMITER, --, quit keywords, backslash commands) then scan the rest - Add Context.Statements to deliver the pre-scanned statements to handlers - IgnoreDelimiterStatements avoids a blocking lookahead read on short complete statements - Add unit tests for the scanner and the reader adapter Refs: #10866
Ctrl-D at the continuation prompt was executing whatever had been typed so far instead of cancelling it. The new uninterpretedReader feeds a live, blocking readLine directly into the StreamScanner, so when readLine returns EOF mid-statement the scanner treated the buffered bytes as a normal EOF-terminated token and ran it. A user that aborts a half typed statement (e.g. an unterminated DELETE) would have it executed instead of cancelled, which diverges from the previous line-accumulation behavior and the mysql client. Add an aborted flag to uninterpretedReader, and set only when a genuine EOF (not a \g and \G special terminator, which also ends the reader via EOF) is reached while pulling a continuation line. When the flag is set, readUninterpretedCommand discards the collected statements and returns io.EOF so the shell's run loop routes to its EOF handler instead of executing the fragment. - Add uninterpretedReader.aborted: set in Read on a genuine EOF from readLine while continuing a command - Check it in readUninterpretedCommand: clear s.statements and return io.EOF instead of the partial command - Add TestUninterpretedReaderAbortedOnContinuationEOF and TestUninterpretedReaderNotAbortedOnSpecialTerminator to pin the discard-on-Ctrl-D behavior and its \g and \G exception Refs: dolthub/dolt#10866
Trim each statement before collecting it into Context.Statements so that entries carry no trailing whitespace or newlines. The scan loop already computed strings.TrimSpace to skip whitespace-only tokens, but then stored the untrimmed token. Store the trimmed value instead. Refs: dolthub/dolt#10866
Restore readMultiLinesFunc to its original func(string) bool predicate and revert its three callers (ReadMultiLinesFunc, read, and the heredoc path in readUninterpreted) to match. The accumulated-buffer argument was left over from the abandoned IsComplete design; after the move to a single streaming scan in readUninterpretedCommand no caller reads it, yet building it called lines.String() on every line, copying the whole buffer each time (quadratic in the number of input lines). The function and its callers are now identical to upstream master, which also shrinks the review surface of the PR. No test updates are needed: no test references the old signature, and go vet and go test ./... pass. Addresses the 2026-08-19 review comment on dolthub#8 about repeated lines.String() calls. Refs: dolthub/dolt#10866
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
1 participant
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.
Summary
Add an opt-in
IsCompletecallback toUninterpretedConfigso consumers can replace the built-in per-lineLineTerminatorsuffix check with their own cumulative-buffer-aware logic. RefactorreadUninterpretedand its accumulation helpers to eliminate duplicate terminator-check code and a duplicate accumulation loop.This is a prerequisite for dolthub/dolt#10866, which fixes four interactive SQL shell bugs that all stem from the per-line suffix check having no awareness of SQL quotes, comments, or escapes.
Consumed by dolthub/dolt#11202, which pins this branch through a temporary
go.modreplaceuntil this PR merges. Replaces #8, which was auto-closed by themastertomainbranch rename.Changes
IsCompletecallback (ishell.go)IsComplete func(accumulated, delimiter string) booltoUninterpretedConfig. When set, it replaces theHasSuffix(line, lineTerminator)test inreadUninterpreted. The callback receives the full cumulative buffer (all lines typed so far, joined by"\n") and the delimiter currently in effect, and returnstruewhen accumulation should stop.Shellstruct asisCompleteand copy it inNewUninterpreted.IsCompletenil see no behavior change; the legacy suffix check runs as before.lineIsCompletehelper (ishell.go)lineIsComplete(line, accumulated string) boolmethod. Special terminators (\g,\G) and backslash commands always end input. Otherwise it defers toisCompletewhen set, or falls back to the originalHasSuffixcheck. This removes the two duplicate terminator-check loops that previously existed in theisComplete != nilandisComplete == nilbranches.Accumulation loop refactor (
ishell.go)readMultiLinesAccumFuncwhose predicate receives both the latest line and the cumulative buffer, enabling theIsCompletecallback to reason over all input so far.readMultiLinesAccumFromReader, the inner loop parameterized on the line-reader and multi-mode toggle functions so it can be driven by unit tests without a real readline instance.readMultiLinesFuncdelegate toreadMultiLinesAccumFunc(ignoring the accumulated argument), removing the previously duplicated loop body.Tests (
uninterpreted_test.go, new file)Four table-driven tests drive
readMultiLinesAccumFromReaderdirectly with canned input lines:TestReadMultiLinesAccumFromReaderTerminator— verifies a cumulative-buffer predicate continues past per-line;and stops on a custom marker.TestReadMultiLinesAccumFromReaderSingleLine— single line that immediately stops; multi-mode is never entered.TestReadMultiLinesAccumFromReaderEmpty— empty line that immediately stops; multi-mode is never entered. Mirrors the empty-Enter behavior in Dolt shell expects more input after empty line whereas MySQL shell returns nothing dolt#10865.TestReadMultiLinesAccumFromReaderEOFMidStream— predicate keeps reading but EOF arrives; surfaces the error with the buffer collected so far.