cli: replace libedit with bubbline #86457
Conversation
|
This change is |
|
Hi @rickystewart @rail ,could I ask for some assistance, as a result of this dep change I am now encountering the following: How can I fix this? |
knz
force-pushed
the
20220819-bubbline
branch
3 times, most recently
from
d696817 to
ab2266c
Compare
|
Remaining to do:
|
|
It looks like Gazelle is confused about this file which is |
|
thank you! |
|
@rickystewart now CI is failing for an interesting reason: Is the requirement to download libedit hard-coded somewhere in the TC configuration? |
Not that I'm aware of, but interestingly I get the same error when I check out your commit too. I'm having a look. |
|
@knz Try |
knz
force-pushed
the
20220819-bubbline
branch
3 times, most recently
from
ca4d12b to
66a70ec
Compare
|
@rickystewart thanks, that worked, and got me one step further. Now I'm running into a different issue: the Is it possible to exclude certain packages from nogo processing? (Also, I'm noticing that nogo is being used for all the CI targets; wouldn't it be more meaningful to only use it for 1 (e.g. the |
knz
force-pushed
the
20220819-bubbline
branch
2 times, most recently
from
828132e to
1d5c7bb
Compare
|
I have downgraded uniseg which removes the error above and also (temporarily) removes the need for a fork. |
knz
force-pushed
the
20220819-bubbline
branch
2 times, most recently
from
b6fbc8d to
b4224c0
Compare
knz
force-pushed
the
20220819-bubbline
branch
from
b4224c0 to
f665389
Compare
|
rebased & ready for review. |
knz
force-pushed
the
20220819-bubbline
branch
from
ef475d9 to
c402940
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @knz)
pkg/cli/clisqlshell/complete.go line 1 at r8 (raw file):
// Copyright 2022 The Cockroach Authors.
Do you think we can add an interactive test for the auto-completion functionality?
pkg/cli/clisqlshell/complete.go line 118 at r8 (raw file):
category := "completions" if len(row) >= 5 { // New-gen server-side completion engine.
I played around with it for a bit but can't see how I can trigger this new-gen completion engine from the cli.
Seems to me that b.sql.runShowCompletions() will always run delegate.RunShowCompletions() that returns a list of completion keywords.
pkg/cli/clisqlshell/editor_bubbline.go line 32 at r8 (raw file):
win, wout, werr *os.File, sqlS sqlShell, maxHistEntries int, histFile string, ) (cleanupFn func(), err error) { cleanupFn = func() {}
nit: this line is never used, remove?
pkg/cli/clisqlshell/editor_bubbline.go line 64 at r8 (raw file):
prevCleanup := cleanupFn cleanupFn = func() { if err := b.ins.SaveHistory(); err != nil {
This is a more general question about cli history: do we clean it up under certain circumstances?
Also, would it be a security vulnerability? Do we consider the case that the history may contain sensitive information?
pkg/cli/clisqlshell/editor_bubbline.go line 116 at r8 (raw file):
// OR: // - or the last token in the input is a semicolon; // and the cursor is either at the end of the input,
I don't quite understand the and the cursor... part here. Does it mean that
CREATE TABLE t (x int);
^
should not work? (as the last token is a semicolon but the cursor is on a space and the position before it is a space too)
pkg/cli/clisqlshell/sql.go line 430 at r8 (raw file):
i, err := strconv.Atoi(v) if err != nil { return err
nit: do we want to wrap the err with hint that please choose from (0 = no, 1 = partial, 2 = full, 3 = extra)?
pkg/cli/clisqlshell/sql.go line 449 at r8 (raw file):
} if i < 0 || i > 1 { return errors.New("value must be between 0 and 1")
nit: value must be 0 for lowercase, or 1 for uppercase
pkg/cli/interactive_tests/test_multiline_statements.tcl line 35 at r8 (raw file):
end_test #start_test "Test that \p does what it says."
Just wanted to confirm -- here and the following commented tests are all pending for certain changes from bubbletea, right?
|
I played around the cool new sql shell and found something, not sure if they are expected:
|
knz
force-pushed
the
20220819-bubbline
branch
from
c402940 to
1091326
Compare
There was a problem hiding this comment.
TFYR, but PTAL.
I did option+shift+? and option+?, none of them worked
"M" stands for meta/alt, not option. Try the alt key.
When I do control+space or control+"-", the shell seems to print out all my shell history with fixed width.
You're entering the key to enable debug mode -- debug mode prints the internal state of the editor continuously, including the history lines. That's what you see. You can press the key again to cancel.
To avoid confusion, i've added a commit to hide this functionality behind --debug-sql-cli
Reviewable status:
pkg/cli/clisqlshell/complete.go line 1 at r8 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
Do you think we can add an interactive test for the auto-completion functionality?
These changes belong to #87606. At the time this PR was created, it wasn't clear which one of the two would be reviewed first, so I made the changes to be independent. I'll extract this bit in a separate PR instead.
pkg/cli/clisqlshell/complete.go line 118 at r8 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
I played around with it for a bit but can't see how I can trigger this new-gen completion engine from the cli.
Seems to me thatb.sql.runShowCompletions()will always rundelegate.RunShowCompletions()that returnsa list of completion keywords.
Again that's #87606. You can test it by running the server from #87606 and the client from this PR.
(But not after this review: I'm removing this code from this PR, will re-add it into a later PR on top of #87606)
pkg/cli/clisqlshell/editor_bubbline.go line 32 at r8 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
nit: this line is never used, remove?
Done.
pkg/cli/clisqlshell/editor_bubbline.go line 64 at r8 (raw file):
do we clean it up under certain circumstances?
Old entries beyond the configuredmaxHistEntriesare removed.
Do we consider the case that the history may contain sensitive information?
It's a regular file. A security-conscious user can simply remove it, or even set the env var COCKROACH_SQL_CLI_HISTORY to an impossible path.
pkg/cli/clisqlshell/editor_bubbline.go line 116 at r8 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
I don't quite understand the
and the cursor...part here. Does it mean thatCREATE TABLE t (x int); ^should not work? (as the last token is a semicolon but the cursor is on a space and the position before it is a space too)
Good call; this comment was stale. Updated, and also added some unit tests for this logic.
pkg/cli/clisqlshell/sql.go line 430 at r8 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
nit: do we want to wrap the err with hint that
please choose from (0 = no, 1 = partial, 2 = full, 3 = extra)?
Good idea. done.
pkg/cli/clisqlshell/sql.go line 449 at r8 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
nit: value must be 0 for lowercase, or 1 for uppercase
Done.
pkg/cli/interactive_tests/test_multiline_statements.tcl line 35 at r8 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
Just wanted to confirm -- here and the following commented tests are all pending for certain changes from bubbletea, right?
Nah, they were stale tests. I removed them entirely.
I meant it stands for alt, not command. |
knz
force-pushed
the
20220819-bubbline
branch
from
55e793c to
01db4cd
Compare
There was a problem hiding this comment.
The mac option key is also called "alt". Try that.
Oh, I was pressing the option (⌥) key on my mac.
I did ⌥+shift+/ and ⌥+/, none of them worked. (?== shift + / IIUC)
I also found that "alt+." is for hide/show prompt, but I tried pressing ⌥ + . and it doesn't work either.
Still a bit confused about this but we can bring this offline.
Reviewable status:
|
I'm curious which terminal app are you using? |
Per an offline convo with @knz , this was solved by having |
knz
force-pushed
the
20220819-bubbline
branch
from
01db4cd to
70fcedd
Compare
knz
force-pushed
the
20220819-bubbline
branch
from
70fcedd to
82e8dc6
Compare
Release note (cli change): The engine used as line editor in the interactive shell (`cockroach sql`, `demo`) has been updated. It includes numerous bug fixes and new features. The previous engine can still be accessed via the env var COCKROACH_SQL_FORCE_LIBEDIT. This support will be removed in a later version.
Release note: None
Release note: None
Release note (cli change): The interactive SQL shell now supports an advanced debug mode for troubleshooting when `--debug-sql-cli` is specified on the command line. The debug mode can be enabled with Ctrl+@ or Ctrl+_ (Ctrl+space on macOS).
knz
force-pushed
the
20220819-bubbline
branch
from
82e8dc6 to
878e76e
Compare
|
bors r=ZhouXing19 |
|
Build succeeded: |
87606: sql: new heuristic-based completion engine r=rytaft,ZhouXing19 a=knz This PR extends the server-side completion logic available under SHOW COMPLETIONS. Fixes #37355. Intended for use together with #86457. The statement now returns 5 columns: completion, category, description, start, end. This change is backward-compatible with the CLI code in previous versions, which was not checking the number of columns. It works roughly as follows: 1. first the input is scanned into a token array. 2. then the token array is translated to a *sketch*: a string with the same length as the token array, where each character corresponds to an input token and an extra character indicating where the completion was requested. 3. then the completion rules are applied in sequence. Each rule inspects the sketch (and/or the token sequence) and decides whether to do nothing (skip to next rule), or to run some SQL which will return a batch of completion candidates. Each method operates exclusively on the token sequence, and does not require the overall SQL syntax to be valid or complete. Also, the completion engine executes in a streaming fashion, so that there is no need to accumulate all the completion candidates in RAM before returning them to the client. This prevents excessive memory usage server-side, and offers the client an option to cancel the search once enough suggestions have been received. Code organization: - new package `sql/compengine`: the core completion engine, also where sketches are computed. Suggested reading: `api.go` in the new package. - new package `sql/comprules`: the actual completion methods/heuristics, where sketches are mapped into SQL queries that provide the completion candidates. Suggested reading: the test cases under `testdata`. Some more words about sketches: For example, `SHOW COMPLETIONS AT OFFSET 10 FOR 'select myc from sc.mytable;'` produces the sketch `"ii'ii.i;"` where `i` indicates an identifier-like token and `'` indicates the cursor position. The purpose of the sketch is to simplify the pattern matching performed by the completion heuristics, and enables the application of regular expressions on the token sequence. For example, a heuristic to complete schema-qualified relations in the current database, or a database-qualified relation, would use the regexp `[^.;]i\.(['_]|i')`: an identifier not following a period or a semicolon, followed by a period, followed by the completion cursor (with the cursor either directly after the period or on an identifier after the period). Supersedes #45186. 93158: dev: fix bep temporary dir path r=rickystewart a=healthy-pod Release note: None Epic: none Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: healthy-pod <[email protected]>
First commit from #88574.
Benefits from (but is not dependent on) #87606 server-side.
Epic: CRDB-22182
Fixes #21826
Fixes #71207
Fixes #71209
Fixes #57885
NB: this will benefit from upstream library releases based off the still-unmerged PRs listed in knz/bubbline#2.
Release justification: n/a will not merge before stability ends
Release note (cli change): The engine used as line editor in the
interactive shell (
cockroach sql,demo) has been updated. Itincludes numerous bug fixes and new features.
The previous engine can still be accessed via the env
var COCKROACH_SQL_FORCE_LIBEDIT. This support will be removed
in a later version.