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

Handle Quotes in Default Column Values #1639

Merged
merged 7 commits into from
Mar 8, 2023

Conversation

macneale4
Copy link
Contributor

@macneale4 macneale4 commented Mar 7, 2023

See Issue: dolthub/dolt#5411

Both Default values and check constraints are impacted by the fact that we write strings to storage without escaping the single quote character or backslash appropriately. This change encodes both and adds tests to verify correct behavior. Tests were added for procedures, views, and triggers, they they weren't impacted by this particular defect.

@macneale4 macneale4 force-pushed the macneale4/literal-string-encoding branch from 7a15c40 to 6810b86 Compare March 7, 2023 16:25
@macneale4 macneale4 marked this pull request as ready for review March 7, 2023 21:26
Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

LGTM! Commented a couple potential supplements

sql/expression/literal.go Show resolved Hide resolved
sql/expression/literal.go Show resolved Hide resolved
@max-hoffman
Copy link
Contributor

Also, this might be a useful one to test on the Dolt side before merging.

@macneale4
Copy link
Contributor Author

I have a bats test in my workspace for dolt that I wasn't convinced was necessary. I can get that in if you think it would be a good idea.

@max-hoffman
Copy link
Contributor

I have a bats test in my workspace for dolt that I wasn't convinced was necessary. I can get that in if you think it would be a good idea.

Nah, the enginetests are sufficient. I just meant the Dolt storage layer can error in novel ways compared to the memory implementation.

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.

2 participants