Skip to content
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: various COPY improvements #82457

Merged
merged 2 commits into from
Jun 13, 2022
Merged

Conversation

otan
Copy link
Contributor

@otan otan commented Jun 6, 2022

refs #41608

Release note: None

@otan otan requested review from rafiss and a team June 6, 2022 03:54
@otan otan requested a review from a team as a code owner June 6, 2022 03:54
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Release note (sql change): COPY ... FROM CSV HEADER is now supported.
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/copy.go line 160 at r2 (raw file):

		if c.format == tree.CopyFormatBinary {
			return nil, pgerror.Newf(
				pgcode.FeatureNotSupported,

PG says "This option is not allowed when using binary format." does PG return FeatureNotSupported here too? i'm thinking InvalidParameter may make more sense


pkg/sql/copy.go line 174 at r2 (raw file):

		if len(delim) != 1 || !utf8.ValidString(delim) {
			return nil, pgerror.Newf(
				pgcode.FeatureNotSupported,

i think PG also doesn't allow this, so ditto


pkg/sql/copy.go line 183 at r2 (raw file):

		if c.format == tree.CopyFormatBinary {
			return nil, pgerror.Newf(
				pgcode.FeatureNotSupported,

ditto

Copy link
Contributor Author

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/copy.go line 160 at r2 (raw file):

otan=# \set VERBOSITY verbose
otan=# copy t from stdin delimiter ',';
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.
>> COPY 0

totally an us problem


pkg/sql/copy.go line 174 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think PG also doesn't allow this, so ditto

otan=# copy t from stdin delimiter 'xx';
ERROR:  0A000: COPY delimiter must be a single one-byte character
LOCATION:  ProcessCopyOptions, copy.c:561

this is correct


pkg/sql/copy.go line 183 at r2 (raw file):

otan=# copy t from stdin null 'xxx';
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.
>> COPY 0

totally an us problem

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/copy.go line 160 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…
otan=# \set VERBOSITY verbose
otan=# copy t from stdin delimiter ',';
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.
>> COPY 0

totally an us problem

but you'd need to test with copy t from stdin binary delimiter ',';


pkg/sql/copy.go line 183 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…
otan=# copy t from stdin null 'xxx';
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.
>> COPY 0

totally an us problem

but what does PG say when you try with binary format?

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/copy.go line 160 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

but you'd need to test with copy t from stdin binary delimiter ',';

just checked -- it's 42601 syntax_error


pkg/sql/copy.go line 174 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…
otan=# copy t from stdin delimiter 'xx';
ERROR:  0A000: COPY delimiter must be a single one-byte character
LOCATION:  ProcessCopyOptions, copy.c:561

this is correct

you're right PG does use 0A000 for this one


pkg/sql/copy.go line 183 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

but what does PG say when you try with binary format?

this is also it's 42601 syntax_error

Copy link
Contributor Author

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/copy.go line 160 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

just checked -- it's 42601 syntax_error

Done.


pkg/sql/copy.go line 183 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this is also it's 42601 syntax_error

ah they must do this at the parser layer. ok

@otan otan requested a review from rafiss June 10, 2022 02:54
@otan
Copy link
Contributor Author

otan commented Jun 13, 2022

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Jun 13, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jun 13, 2022

Build succeeded:

@craig craig bot merged commit 87c0088 into cockroachdb:master Jun 13, 2022
@rafiss rafiss mentioned this pull request Aug 2, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants