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/sem/tree: support parsing of tuples from string literals #71916

Merged
merged 2 commits into from
Oct 29, 2021

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Oct 25, 2021

fixes #70030
fixes #63995

Release note (sql change): String literals can now be parsed as tuples,
either in a cast expression, or in other contexts like function
arguments.

Release note (bug fix): Previously, when records and enums containing
escape sequences were shown in the CLI, they would be incorrectly
double-escaped. This is now fixed.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss requested a review from a team October 25, 2021 05:33
Copy link
Contributor

@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.

looks good!

out of pure curiousity, did you copy postgres' parsing logic or is this rafi proprietary code? if the former it might be worth putting a code link in there

pkg/sql/sem/tree/format_test.go Outdated Show resolved Hide resolved
Datums{NewDTuple(tupleOfStringAndInt, NewDString("a12'"), NewDInt(4))},
},
{
`{"(cat,4)", "(null,0)"}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to test non-int/text types, e.g. timestamptz

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do. i need to iron out some of the random test issues too

return nil, false, malformedRecordError
}
if parser.tupleIdx < len(parser.t.TupleContents()) {
return nil, false, errors.WithDetail(malformedRecordError, "Too few columns.")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: errors should not have periods and start with lower case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the error detail though. i think we do use full sentences for that?

err = errors.WithDetail(err, `The prefix "pg_" is reserved for system schemas.`)

Copy link
Contributor

Choose a reason for hiding this comment

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

can we be more detailed (i.e. print out expected types)?
should we do that for the too many columns case?

@rafiss rafiss force-pushed the string-tuple-parsing branch from 541b870 to 5c20242 Compare October 28, 2021 19:47
@blathers-crl blathers-crl bot requested a review from otan October 28, 2021 19:47
@rafiss rafiss marked this pull request as ready for review October 28, 2021 19:48
@rafiss rafiss requested a review from a team as a code owner October 28, 2021 19:48
@rafiss
Copy link
Collaborator Author

rafiss commented Oct 28, 2021

This is ready for a full review. I ran with randomTupleIterations=1000000 and it passed so I feel OK about it.

@rafiss rafiss requested a review from a team October 28, 2021 19:49
Copy link
Contributor

@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.

lgtm, one optional comment and you have some test cases to change

return nil, false, malformedRecordError
}
if parser.tupleIdx < len(parser.t.TupleContents()) {
return nil, false, errors.WithDetail(malformedRecordError, "Too few columns.")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we be more detailed (i.e. print out expected types)?
should we do that for the too many columns case?

@rafiss
Copy link
Collaborator Author

rafiss commented Oct 28, 2021

can we be more detailed (i.e. print out expected types)?
should we do that for the too many columns case?

the MakeParseError function does that already

@rafiss rafiss force-pushed the string-tuple-parsing branch from 5c20242 to 8ece1b5 Compare October 28, 2021 21:50
Release note (sql change): String literals can now be parsed as tuples,
either in a cast expression, or in other contexts like function
arguments.
Release note (bug fix): Previously, when records and enums containing
escape sequences were shown in the CLI, they would be incorrectly
double-escaped. This is now fixed.
@rafiss rafiss force-pushed the string-tuple-parsing branch from 8ece1b5 to a48d5ab Compare October 29, 2021 05:01
@rafiss
Copy link
Collaborator Author

rafiss commented Oct 29, 2021

bors r=otan

@craig
Copy link
Contributor

craig bot commented Oct 29, 2021

Build failed:

@rafiss
Copy link
Collaborator Author

rafiss commented Oct 29, 2021

bors r=otan

@craig
Copy link
Contributor

craig bot commented Oct 29, 2021

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.

sql: support string literal -> tuple[] parsing tree: fix error message for parsing tuples in arrays
3 participants