Add opt-in IsComplete callback to UninterpretedConfig - #8
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
| // readMultiLinesAccumFromReader is the inner accumulation loop, parameterized | ||
| // on the line reader and multi-mode toggle so tests can drive it without a real | ||
| // readline instance. | ||
| func readMultiLinesAccumFromReader( |
There was a problem hiding this comment.
why does this need to be its own function when it's never directly called and only ever called from s.readMultiLinesAccumFunc?
There was a problem hiding this comment.
@angelamayxie I added readMultiLinesAccumFromReader only so I could unit-test the accumulation loop without a real readline/TTY. But that loop is unchanged pre-existing behavior; the only new thing in this PR is passing the accumulated buffer to the predicate, and that path is already covered end-to-end by the BATS tests in the Dolt PR. So, I can put everything back into readMultiLinesFunc change its predicate to func(line, accumulated string) bool, update the other callers (i.e., read() and the heredoc path) to ignore the second arg, and drop the standalone loop tests. Does that sound ok given the tests in the Dolt PR?
There was a problem hiding this comment.
That works -- it doesn't need to be unit tested here since it gets tested anyways in the Dolt BATS tests.
| // readMultiLinesAccumFunc accumulates physical lines until the predicate | ||
| // returns false. The predicate receives the latest line and the cumulative | ||
| // buffer (lines joined by "\n"). | ||
| func (s *Shell) readMultiLinesAccumFunc(f func(line, accumulated string) (keepReading bool)) (string, error) { |
There was a problem hiding this comment.
I don't think a new function is necessary. the original readMultiLinesFunc and calls to it can just be modified.
| line, err = readLine() | ||
| fmt.Fprint(&lines, line) | ||
| if !f(line) || err != nil { | ||
| if !f(line, lines.String()) || err != nil { |
There was a problem hiding this comment.
repeatedly calling lines.String() to get the accumulated string does not seem performant to me. Also, looking at how it's integrated with Dolt, it seems like everything is getting scanned twice, first via the iShell and then again with the sql scanner. And if it's multiple lines, then the first line gets scanned n times, second line n-1 times, and so on? Ideally, the input should just get scanned once.
|
hi @angelamayxie FYI, sorry for the delay. I'll respond to your review comments this evening after work. |
codeaucafe
left a comment
codeaucafe
left a comment
There was a problem hiding this comment.
Left a comment. Regarding your inefficient scanning twice comment I've made a corresponding comment in the Dolt PR you made since your Dolt comment is related to this inefficient scanning.
| // readMultiLinesAccumFromReader is the inner accumulation loop, parameterized | ||
| // on the line reader and multi-mode toggle so tests can drive it without a real | ||
| // readline instance. | ||
| func readMultiLinesAccumFromReader( |
There was a problem hiding this comment.
@angelamayxie I added readMultiLinesAccumFromReader only so I could unit-test the accumulation loop without a real readline/TTY. But that loop is unchanged pre-existing behavior; the only new thing in this PR is passing the accumulated buffer to the predicate, and that path is already covered end-to-end by the BATS tests in the Dolt PR. So, I can put everything back into readMultiLinesFunc change its predicate to func(line, accumulated string) bool, update the other callers (i.e., read() and the heredoc path) to ignore the second arg, and drop the standalone loop tests. Does that sound ok given the tests in the Dolt PR?
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
9a52337 to
950b93b
Compare
2 participants
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.
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.