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: also handle ESCAPE characters properly in COPY machine #86929

Merged
merged 1 commit into from
Aug 27, 2022

Conversation

otan
Copy link
Contributor

@otan otan commented Aug 26, 2022

Release justification: critical bug fix to existing functionality

Release note (bug fix): Previously, escaping a double quote (") with
COPY in CSV mode could ignore all subsequent lines in the same COPY
if an ESCAPE clause were specified. This is now resolved.

@otan otan requested review from rafiss and a team August 26, 2022 04:52
@otan otan requested a review from a team as a code owner August 26, 2022 04:52
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@ajstorm ajstorm 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 538 at r1 (raw file):

		} else {
			// Otherwise, we have to do a more involved count of quotes and
			// ignore any escaped quotes for counting.

Nit: Can we build out this comment a bit more, perhaps with a short example? It took me a bit to understand what's going on below, and I worry others who encounter this code for the first time will also be confused.


pkg/sql/copy.go line 541 at r1 (raw file):

			isEscaped := false
			for _, ch := range line {
				if isEscaped {

Nit: I'm wondering if this would be more readable if you called this variable skipNextChar and then added a comment below saying that "We found a rune which matches the specified escape value. This means we need to skip the next character, as it's been escaped"?

Release justification: critical bug fix to existing functionality

Release note (bug fix): Previously, escaping a double quote (`"`) with
`COPY` in `CSV` mode could ignore all subsequent lines in the same COPY
if an `ESCAPE` clause were specified. This is now resolved.
Copy link
Collaborator

@ajstorm ajstorm left a comment

Choose a reason for hiding this comment

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

This :lgtm: now. I'm wondering if as a follow on item in stability we should add far more robust (randomized?) testing for parsing in COPY. I worry that this may not be the only bug lingering.

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

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.

nice find. i don't think backports are needed since v22.2 is the first release where ESCAPE is supported.

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

@rafiss
Copy link
Collaborator

rafiss commented Aug 26, 2022

oh, i forgot that we backported ESCAPE

@daniel-crlabs
Copy link
Contributor

This affects customers on cockroach cloud as well, see linked ticket.

@otan
Copy link
Contributor Author

otan commented Aug 27, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 27, 2022

Build succeeded:

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.

5 participants