-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql: implement COPY ... TO STDOUT for CSV & TEXT #94408
Conversation
a9d123f
to
f2018d2
Compare
75f2d17
to
7f24c54
Compare
8959f47
to
b286353
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super cool! thanks for doing this!!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @otan)
-- commits
line 20 at r3:
how does FormatWireText
differ from FmtPgwireText
or fmtPgwireFormat
-- commits
line 48 at r6:
you could save the release note for this last one, and have it mention CSV support.
pkg/cli/clisqlshell/sql.go
line 1878 at r5 (raw file):
} var copyToRe = regexp.MustCompile(`(?i)COPY.*TO\s+STDOUT`)
it seems possible that this could have a false positive match if COPY FROM is used with a table named "TO STDOUT" but maybe it doesn't matter..
pkg/sql/pgwire/conn.go
line 1298 at r1 (raw file):
case tree.CopyIn, tree.CopyOut: // Nothing to do. The CommandComplete message has been sent elsewhere. panic(errors.AssertionFailedf("CopyIn statements should have been handled elsewhere " +
nit: update the error message
pkg/sql/pgwire/conn.go
line 882 at r4 (raw file):
if cp, ok := stmts[i].AST.(*tree.CopyTo); ok { if len(stmts) != 1 { // NOTE(andrei): I don't know if Postgres supports receiving a COPY
does andrei's note apply to COPY TO? (the "taking over the connection" part)
pkg/sql/sem/tree/copy.go
line 26 at r1 (raw file):
} // CopyTo represents a COPY FROM statement.
nit: update the comment
pkg/sql/copy/testdata/copy_to_binary
line 16 at r4 (raw file):
COPY t TO STDOUT BINARY ---- ERROR: unimplemented: binary format for COPY TO not implemented (SQLSTATE 0A000)
does the issue link show up too?
is it because the issue is only in the hint? i wonder if we should just be putting them into the message so more people see them. (just wondering; that would be separate from this PR)
pkg/sql/copy/testdata/copy_to_text
line 2 at r4 (raw file):
exec-ddl CREATE TABLE t (id int primary key, t text);
i'd be curious to see how COPY TO handles weird stuff like the bytea
type, arrays, tuples, time zones.
pkg/sql/parser/testdata/copy
line 113 at r1 (raw file):
COPY t TO 'file' ^ HINT: You have attempted to use a feature that is not yet implemented.
is there a way we can get an unimplementedWithIssue error for this?
pkg/sql/copy_out_test.go
line 85 at r4 (raw file):
// TestCopyOutRandom tests COPY TO on a set of random rows matches pgx. func TestCopyOutRandom(t *testing.T) {
cool test!
pkg/sql/copy_out_test.go
line 99 at r4 (raw file):
colTypes := []*types.T{types.Int} colStr := "id INT PRIMARY KEY" for _, typ := range types.Scalar {
can non-scalar types be tested here too somehow?
pkg/sql/pgwire/types_test.go
line 44 at r3 (raw file):
// TestWriteTestDatumMatchesFormatWireText is required so long as writeTextDatum // has a separate implementation to tree.Datum.FormatWireText. func TestWriteTestDatumMatchesFormatWire(t *testing.T) {
should it be named TestWriteTextDatumMatchesFormatWire
instead?
pkg/sql/pgwire/types_test.go
line 50 at r3 (raw file):
defaultConv, defaultLoc := makeTestingConvCfg() rng := rand.New(rand.NewSource(0))
nit: use randutil.NewTestRand
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems great! thanks for doing it
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 11 files at r12.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @knz)
This commit sets up the parsing required for COPY TO. Release note: None
Strip out copy options to a function ready for re-use from the copy machine in preparation for COPY TO. Release note: None
Release note: None
This commit enables the CLI to correctly route COPY TO commands to the server. Release note: None
This commit introduces the relevant CSV format components for COPY TO. Release note (sql change): Implemented the `COPY ... TO STDOUT` statement, which allows exporting a table or arbitrary query using the postgres wire-compatible format. Only text & CSV format is supported.
Release note: None
bors r=rafiss |
Build failed (retrying...): |
Build failed (retrying...): |
Build failed (retrying...): |
bors r=rafiss |
Build failed: |
bors is dying, flakes are alive bors r=rafiss |
Build succeeded: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/cli/clisqlshell/sql.go
line 1974 at r18 (raw file):
Previously, otan (Oliver Tan) wrote…
not sure how much it helps here:
root@127.0.0.1:26257/defaultdb> show syntax 'copy t to stdout'; field | message --------+------------------- sql | COPY t TO STDOUT (1 row) Time: 0ms total (execution 0ms / network 0ms)
For one, your thing does not handle string literals nor SQL comments.
Second, you're using scanner.FirstLexicalToken
so you could actually decompose the tokens further and inspect them -- check that there are just 4 tokens, and the last 2 are "TO" and "STDOUT".
Third we could have a variant of SHOW SYNTAX that calls the (new) parser.Tokens()
under the hood as that would do the tokenization server-side and give you the information you need.
This needs to be addressed -- we can't have a precedent of folk using regular expressions for SQL, as SQL is not a regular language.
filed #97508 |
See individual commits for details.
These commits do not implement
COPY ... TO STDOUT BINARY
. That can be a later exercise!fixes #85571