sqlcli,cdc: improve sinkless changefeed display in interactive sql shell#85181
Conversation
|
This change is |
|
What about having the cli detect a sinkless changefeed statement and then switch the mode automatically? |
I do think that'd be reasonable but I like the idea of being backwards-compatible. Even gating behind Still, we could just add a setting to turn the automatic mode switching off. And maybe it'd be better to be parsing the statement to check if it's sinkless so that we can support changefeed syntax that doesn't work with |
06cb8db to
ac2aae2
Compare
This is far-fetched and something we can freely change in a major version CLI release. |
11318bd to
49d94b3
Compare
|
Pushed a commit that detects a sinkless changefeed statement and switches the mode automatically. |
c7491db to
9ee9ee1
Compare
rafiss
left a comment
There was a problem hiding this comment.
i'd be in favor of just going with commit 2 (or i guess, squash these two together). i like the tests, but just had some questions
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @rafiss)
pkg/cli/clisqlshell/parser.go line 18 at r2 (raw file):
) // We don't depend on tree in this package, but we do
i worry a bit about this being hard to maintain, since the CLI doesn't see much active development.
do you have thoughts on doing regexp matching instead? or do what we do for COPY and be a bit more naive?
cockroach/pkg/cli/clisqlshell/sql.go
Line 1794 in 47dfddf
pkg/cli/clisqlshell/parser.go line 35 at r2 (raw file):
// an undefined transition: the token is not in // transitions and defaultTransition is unset. type statementType struct {
is statementType the right name here? it seems like it's more like tokenType?
pkg/cli/clisqlshell/sql.go line 1172 at r2 (raw file):
// record values are output as bytea datums, and escaped output // is much more readable than the default hex. if prevByteaOutput == `hex` {
should this be if prevByteaOutput != escape`?
HonoreDB
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cli/clisqlshell/parser.go line 18 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i worry a bit about this being hard to maintain, since the CLI doesn't see much active development.
do you have thoughts on doing regexp matching instead? or do what we do for
COPYand be a bit more naive?cockroach/pkg/cli/clisqlshell/sql.go
Line 1794 in 47dfddf
I'm open to doing this more naively. The complexity mainly comes from checking whether there's an INTO clause; I don't want anyone to think their changefeed on TABLE tennis is fine, but a changefeed on table badminton hangs, and then be annoyed hours later to discover that the problem is that badminton contains "into". But false positives seem like they wouldn't be a big deal. I could change it to just always switch modes if the second lexical token is CHANGEFEED, and if you're creating a sinkful changefeed you'll get the job ID back in json form instead of table form, so it'll look like
{"job_id":"787032705335984129"}
instead of
job_id
----------------------
787032804510957569
What do you think?
pkg/cli/clisqlshell/parser.go line 35 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
is
statementTypethe right name here? it seems like it's more liketokenType?
It's both, right now. If I'm not deleting this file entirely I'll make those distinct types.
pkg/cli/clisqlshell/sql.go line 1172 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
should this be
if prevByteaOutput !=escape`?
I think no? Right now it makes no difference since those are the only two possible values, but presumably if another value is added it won't be the default, and if you've explicitly set your bytea_output to something other than the default you know what you're doing and don't want it overridden/
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cli/clisqlshell/parser.go line 18 at r2 (raw file):
I could change it to just always switch modes if the second lexical token is CHANGEFEED, and if you're creating a sinkful changefeed you'll get the job ID back in json form instead of table form
that seems fine to me.
pkg/cli/clisqlshell/sql.go line 1172 at r2 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
I think no? Right now it makes no difference since those are the only two possible values, but presumably if another value is added it won't be the default, and if you've explicitly set your bytea_output to something other than the default you know what you're doing and don't want it overridden/
i see, that sounds fine
9ee9ee1 to
209f796
Compare
This commit improves the experience of someone trying out CREATE CHANGEFEED or EXPERIMENTAL CHANGEFEED in a cockroach sql terminal by automatically turning off the table display format (which prevents flushing) and setting bytea_output=escape, as core changefeeds output JSON as bytes datums, for the duration of the changefeed query. Changefeeds with sinks specified are not affected, nor are cockroach sql invocations that don't detectably output to a terminal. Release note (cli change): Improved the output of sinkless changefeeds in the cockroach sql terminal. Release justification: Prevents UX degradation in 22.2 as a side effect of fixing the bytea_output bug.
209f796 to
fb09e01
Compare
HonoreDB
left a comment
There was a problem hiding this comment.
Squashed!
Reviewable status:
pkg/cli/clisqlshell/parser.go line 18 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
I could change it to just always switch modes if the second lexical token is CHANGEFEED, and if you're creating a sinkful changefeed you'll get the job ID back in json form instead of table form
that seems fine to me.
Per offline discussion, keeping as-is with the promise that CDC team is on the hook for maintaining this.
pkg/cli/clisqlshell/parser.go line 35 at r2 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
It's both, right now. If I'm not deleting this file entirely I'll make those distinct types.
Fixed.
pkg/cli/clisqlshell/sql.go line 1172 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i see, that sounds fine
Done.
rafiss
left a comment
There was a problem hiding this comment.
lgtm!
Reviewable status:
|
TFTR! bors r=[rafiss] |
|
Build succeeded: |
This commit improves the experience of someone
trying out CREATE CHANGEFEED or EXPERIMENTAL
CHANGEFEED in a cockroach sql terminal by
automatically turning off the table display
format (which prevents flushing) and setting
bytea_output=escape, as core changefeeds output
JSON as bytes datums, for the duration of the
changefeed query.
Changefeeds with sinks specified are not affected,
nor are cockroach sql invocations that don't
detectably output to a terminal.
Release note (cli change): Improved the output of sinkless changefeeds in the cockroach sql terminal.
Release justification: Prevents UX degradation in 22.2 as a side effect of fixing the bytea_output bug.