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: fix cast from string to array with width #50153

Merged
merged 1 commit into from
Jun 13, 2020

Conversation

RaduBerinde
Copy link
Member

The code that performs a cast from string to array doesn't take into account the
width of the string type it is producing. This doesn't show up when the input is
a literal constant because the constant is first parsed as an array of (vanilla)
strings which is then subjected to a cast (to array of strings with width).

Fixes #50132.

Release note (bug fix): Fixed some cases of casting a string to a width-limited
string array.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

wondering if we should backport this in case people relied on this behaviour in older versions (i hope not?!).

// If the string type specifies a limit we truncate to that limit:
// 'hello'::CHAR(2) -> 'he'
// This is true of all the string type variants.
if t.Width() > 0 && int(t.Width()) < len(s) {
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting that parsing and storing strings that are too long have different results.

otan=# create temp table a (a varchar(1));
CREATE TABLE

otan=# insert into a values ('123');
ERROR:  value too long for type character varying(1)

Should this be rune count like we do here:

if typ.Width() > 0 && utf8.RuneCountInString(sv) > int(typ.Width()) {
?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. I copied this from PerformCast (string->string case). I think you're right though, postgres docs say that the limit is n "characters (not bytes)".

@RaduBerinde
Copy link
Member Author

I will open a separate PR to fix the unicode stuff. And yeah, we should backport both (I already added the backport labels).

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.

ok! lgtm!

@RaduBerinde
Copy link
Member Author

Opened #50156. I will rebase this on top of that.

The code that performs a cast from string to array doesn't take into
account the width of the string type it is producing. This doesn't
show up when the input is a literal constant because the constant is
first parsed as an array of (vanilla) strings which is then subjected
to a cast (to array of strings with width).

Fixes cockroachdb#50132.

Release note (bug fix): Fixed some cases where casting a string to a
width-limited string array was not truncating the string.
@RaduBerinde
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 13, 2020

Build succeeded

@craig craig bot merged commit 72d1ec4 into cockroachdb:master Jun 13, 2020
@RaduBerinde RaduBerinde deleted the fix-strarr-cast branch June 16, 2020 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: cast from string to array doesn't take into account type width
3 participants