sql: various COPY improvements #78303

otan · 2022-03-23T04:06:10Z

Refs: #41608

See individual commits for details.

Release note: None

cockroach-teamcity · 2022-03-23T04:06:22Z

This change is

Release note: None

ajstorm

Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 3 of 4 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @otan)

-- commits, line 7 at r2:
Can we add some testing here to validate that the options show up as unimplemented?

pkg/util/encoding/csv/reader_test.go, line 396 at r4 (raw file):

		Escape: 'x',
		Input:  `"x"",",","xxx"",x,"xxxx,"` + "\n",
		Output: [][]string{{`"`, `,`, `x"`, `x`, `xx,`}},

I'm not following the 4th case. If we just provide an escape character, shouldn't this show nothing or a parse error?

pkg/util/encoding/csv/reader_test.go, line 402 at r4 (raw file):

		Comma:  'x',
		Input:  `"x""x,x"xxx""x"xx"x"xxxx,"` + "\n",
		Output: [][]string{{`"`, `,`, `x"`, `x`, `xx,`}},

I think I'm missing something. I would have thought that this would yield:

'"',',"x"','"x','"xx'

pkg/util/encoding/csv/writer_test.go, line 54 at r4 (raw file):

	{Input: [][]string{{"a", "a", "a"}}, Output: "a,a,a\n"},
	{Input: [][]string{{`\.`}}, Output: "\"\\.\"\n"},
	{Input: [][]string{{`"`, `,`, `x"`, `x`, `xx,`}}, Escape: 'x', Output: `"x"",",","xxx"",x,"xxxx,"` + "\n"},

More confusion on my part- why is a single x being interpreted as x instead of as the escape, and thus a parse error?

awoods187 · 2022-03-23T18:39:51Z

Exciting stuff--can we list explicitly what will remain unimplemented for Copy from the linked issue?

ajstorm · 2022-03-23T19:20:44Z

It's everything in #41608 except for the ESCAPE option.

…

On Wed, Mar 23, 2022 at 2:40 PM Andy Woods ***@***.***> wrote: Exciting stuff--can we list explicitly what will remain unimplemented for Copy from the linked issue? — Reply to this email directly, view it on GitHub <#78303 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEMXVOUO6T6PTK53IU6FD6DVBNQQFANCNFSM5RMY66CA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

awoods187 · 2022-03-23T19:26:50Z

Awesome news!

otan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm)

-- commits, line 7 at r2:

Previously, ajstorm (Adam Storm) wrote…

Can we add some testing here to validate that the options show up as unimplemented?

the new tests in TestUnimplementedSyntax in parse_test.go already does this

pkg/util/encoding/csv/reader_test.go, line 396 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

I'm not following the 4th case. If we just provide an escape character, shouldn't this show nothing or a parse error?

"x"",",","xxx"",x,"xxxx,"` shouldn't.
breaking it down:

"x"",: "x"" - the wrapping " indicates to use escapes, x escapes ", so it is ".
",",: the "," is just ,
"xxx"",: the x escapes x, then x escapes ", so it is x"
x,: no quotes, so we don't have to escape anything. so it is x (see below comment)
"xxxx,", x escapes x twice, then a comma, so xx,

Code quote:

"x"",",","xxx"",x,"xxxx,"` + "\n"

pkg/util/encoding/csv/reader_test.go, line 402 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

I think I'm missing something. I would have thought that this would yield:

'"',',"x"','"x','"xx'

"x""x, yields ", then a delimiter
,x yields , then a delimiter
"xxx""x yields "x", then a delimiter "xx"x": point of contention. begin quote, then x escapes x, so it becomes just x, then a delimiter
"xxxx,": quote, two escapes x's, then a comma, so xx,

fwiw all these outputs are from PG

pkg/util/encoding/csv/writer_test.go, line 54 at r4 (raw file):

Previously, ajstorm (Adam Storm) wrote…

More confusion on my part- why is a single x being interpreted as x instead of as the escape, and thus a parse error?

because things that are not in quotes are their accepted "raw value".

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @otan)

pkg/sql/pgwire/testdata/pgtest/copy, line 297 at r5 (raw file):

{"Type":"DataRow","Values":[{"text":"1"},{"text":"x\",x"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 2"}
{"Type":"ReadyForQuery","TxStatus":"I"}

just to double-check, these new ones also passed against PG?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @otan)

pkg/util/encoding/csv/reader.go, line 133 at r4 (raw file):

	// Escape, if not 0, is the character used to escape `"` characters and
	// itself.

maybe mention that if it's unset, it defaults to the same as QUOTE. (which will only be more important once we support the QUOTE option)

otan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @otan, and @rafiss)

pkg/util/encoding/csv/reader.go, line 133 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

maybe mention that if it's unset, it defaults to the same as QUOTE. (which will only be more important once we support the QUOTE option)

Done.

rafiss

looks good from my end

ajstorm

LGTM too. Thanks for clearing up my confusion about the unquoted characters.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @otan, and @rafiss)

-- commits, line 7 at r2:

Previously, otan (Oliver Tan) wrote…

the new tests in TestUnimplementedSyntax in parse_test.go already does this

Thanks. I see them now.

pkg/util/encoding/csv/reader_test.go, line 396 at r4 (raw file):

Previously, otan (Oliver Tan) wrote…

"x"",",","xxx"",x,"xxxx,"` shouldn't.
breaking it down:

"x"",: "x"" - the wrapping " indicates to use escapes, x escapes ", so it is ".
",",: the "," is just ,
"xxx"",: the x escapes x, then x escapes ", so it is x"
x,: no quotes, so we don't have to escape anything. so it is x (see below comment)
"xxxx,", x escapes x twice, then a comma, so xx,

Yeah, makes sense. It was step 4 that I was missing.

pkg/util/encoding/csv/reader_test.go, line 402 at r4 (raw file):

Previously, otan (Oliver Tan) wrote…

"x""x, yields ", then a delimiter
,x yields , then a delimiter
"xxx""x yields "x", then a delimiter "xx"x": point of contention. begin quote, then x escapes x, so it becomes just x, then a delimiter
"xxxx,": quote, two escapes x's, then a comma, so xx,

fwiw all these outputs are from PG

All good. Thanks.

pkg/util/encoding/csv/writer_test.go, line 54 at r4 (raw file):

Previously, otan (Oliver Tan) wrote…

because things that are not in quotes are their accepted "raw value".

Ahh, interesting. That explains all of my confusion. Thx.

Add `ESCAPE` logic to the `encoding/csv` package, for exposure to SQL at a later stage. It is worth noting I wrote this in the "safest" backportable way possible. Ideally we'd rewrite the read logic to be more "parser"-like to account for the change in QUOTE case, but that's a lot riskier to backport. Release note: None

Release note (sql change): Implemented the `COPY FROM ... ESCAPE ...` syntax.

otan · 2022-03-24T03:50:17Z

thanks

bors r=rafiss,ajstorm

craig · 2022-03-24T04:28:30Z

Build succeeded:

GitHub CI (Cockroach)

otan · 2022-03-24T13:06:31Z

blathers backport 22.1

…

On Thu, 24 Mar 2022, 3:28 pm craig[bot], ***@***.***> wrote: Merged #78303 <#78303> into master. — Reply to this email directly, view it on GitHub <#78303 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA32FQ64NXUT2VWTPFBIYLTVBPVP7ANCNFSM5RMY66CA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

otan · 2022-03-24T13:13:26Z

blathers backport 21.2

otan · 2022-06-08T21:43:57Z

blathers backport 21.2

blathers-crl · 2022-06-08T21:44:09Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 149a1db to blathers/backport-release-21.2-78303: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

otan added 2 commits March 23, 2022 12:15

sql: amend COPY to use CRDB csv implementation

7daca70

Release note: None

parser: add unimplemented syntax for all the COPY options

149a1db

Release note: None

otan requested review from ajstorm and a team March 23, 2022 04:06

otan requested a review from a team as a code owner March 23, 2022 04:06

tree: add pgcodes to multiple specified COPY options

a503539

Release note: None

otan force-pushed the csv_editing branch from 923d925 to e151af5 Compare March 23, 2022 04:29

ajstorm requested changes Mar 23, 2022

View reviewed changes

ajstorm self-requested a review March 23, 2022 15:29

otan commented Mar 23, 2022

View reviewed changes

otan force-pushed the csv_editing branch 2 times, most recently from 735456b to e690e37 Compare March 23, 2022 20:28

rafiss reviewed Mar 23, 2022

View reviewed changes

otan force-pushed the csv_editing branch from e690e37 to c592b2e Compare March 23, 2022 21:41

otan commented Mar 23, 2022

View reviewed changes

rafiss approved these changes Mar 23, 2022

View reviewed changes

ajstorm requested a review from rafiss March 23, 2022 23:42

ajstorm approved these changes Mar 23, 2022

View reviewed changes

ajstorm self-requested a review March 23, 2022 23:42

otan force-pushed the csv_editing branch 3 times, most recently from f102228 to 03ee50c Compare March 24, 2022 00:51

otan added 2 commits March 24, 2022 11:54

sql: implement COPY FROM ... ESCAPE ...

7e2c7f4

Release note (sql change): Implemented the `COPY FROM ... ESCAPE ...` syntax.

otan force-pushed the csv_editing branch from 03ee50c to 7e2c7f4 Compare March 24, 2022 00:54

craig bot merged commit 8b36717 into cockroachdb:master Mar 24, 2022

cockroach-teamcity mentioned this pull request Mar 24, 2022

sql: various COPY improvements cockroachdb/docs#13330

Closed

blathers-crl bot mentioned this pull request Mar 24, 2022

release-22.1: sql: various COPY improvements #78417

Merged

ajstorm mentioned this pull request Mar 28, 2022

Postgres compatibility for AWS DMS #34766

Closed

otan mentioned this pull request Jun 8, 2022

release-21.2: sql: various COPY improvements #82636

Merged

rafiss mentioned this pull request Aug 2, 2022

sql: support more COPY syntax #41608

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: various COPY improvements #78303

sql: various COPY improvements #78303

otan commented Mar 23, 2022

cockroach-teamcity commented Mar 23, 2022

ajstorm left a comment

awoods187 commented Mar 23, 2022

ajstorm commented Mar 23, 2022 via email

awoods187 commented Mar 23, 2022

otan left a comment

rafiss left a comment

rafiss left a comment

otan left a comment

rafiss left a comment

ajstorm left a comment

otan commented Mar 24, 2022

craig bot commented Mar 24, 2022

otan commented Mar 24, 2022 via email

otan commented Mar 24, 2022

otan commented Jun 8, 2022

blathers-crl bot commented Jun 8, 2022

sql: various COPY improvements #78303

sql: various COPY improvements #78303

Conversation

otan commented Mar 23, 2022

cockroach-teamcity commented Mar 23, 2022

ajstorm left a comment

Choose a reason for hiding this comment

awoods187 commented Mar 23, 2022

ajstorm commented Mar 23, 2022 via email

awoods187 commented Mar 23, 2022

otan left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

otan commented Mar 24, 2022

craig bot commented Mar 24, 2022

otan commented Mar 24, 2022 via email

otan commented Mar 24, 2022

otan commented Jun 8, 2022

blathers-crl bot commented Jun 8, 2022