-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
cli/sql: properly format common array types #66079
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! just had two related comments
pkg/cli/cli_test.go
Outdated
// {哈哈} {哈哈} {哈哈} | ||
// sql -e select array['哈哈'::CHAR(2)], array['哈哈'::"char"] | ||
// array array | ||
// {哈哈} {哈哈} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vs := formatVal(arrayVal, colType, showPrintableUnicode, showNewLinesAndTabs) | ||
// Add the string for that one value to the output array representation. | ||
buf.WriteString(vs) | ||
comma = "," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the strings inside of a string array already correctly escaped if they have commas in them? i.e., the array element should then be surrounded with double quotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @rafiss)
pkg/cli/cli_test.go, line 337 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
this is separate from your PR, but that
"char"
one is supposed to get truncated. #65631if the test is left as-is, it'll need to be fixed when #65631 is addressed, or you could change this test right now so the
"char"
value is just one character long. either way is fine; up to you.
Good idea. Done.
pkg/cli/sql_util.go, line 1297 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
are the strings inside of a string array already correctly escaped if they have commas in them? i.e., the array element should then be surrounded with double quotes
Ah! Good call. There was a bug there. Fixed the bug and added tests for it.
Release note (bug fix): CockroachDB's SQL shell now properly displays results of common array types, for example arrays of floats or arrays of strings.
TFYR! bors r=rafiss |
Build failed: |
unrelated flake bors r=rafiss |
Build succeeded: |
66116: cli: split cli_test.go into separate files r=otan a=knz First commit from #66079. cc @cockroachdb/server 66167: Revert "teamcity-support: make github-post best effort" r=tbg a=otan don't think it was the problem, reverting back to old behaviour as i think CI should be red if we had no corresponding github issue for it. ---- This reverts commit 8bfad1d. Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Oliver Tan <[email protected]>
Fixes #43994
Fixes #31872.
Release note (bug fix): CockroachDB's SQL shell now properly displays
results of common array types, for example arrays of floats or
arrays of strings.