-
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
sql: propagate bytea_output to distsql processors #27951
Conversation
d7e574c
to
d7351ba
Compare
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.
Reviewed 6 of 6 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/distsqlrun/server.go, line 341 at r1 (raw file):
} var be sessiondata.BytesEncodeFormat
You do this more than once. DRY it up?
pkg/sql/distsqlrun/server.go, line 360 at r1 (raw file):
SearchPath: sessiondata.MakeSearchPath(req.EvalContext.SearchPath), SequenceState: sessiondata.NewSequenceState(), BytesEncodeFormat: be,
If you're alphabetizing location, why not move bytesencodeformat as well.
pkg/sql/logictest/testdata/logic_test/bytes, line 122 at r1 (raw file):
SELECT e'a\\01'::STRING::BYTEA # Regression test for #27950.
make this a subtest and drop the table at the end of it
d7351ba
to
c621d25
Compare
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 (and 1 stale)
pkg/sql/distsqlrun/server.go, line 341 at r1 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
You do this more than once. DRY it up?
Nope, these are two different patterns (conversion in opposite directions).
pkg/sql/distsqlrun/server.go, line 360 at r1 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
If you're alphabetizing location, why not move bytesencodeformat as well.
Nope, because the follow-up PR #27952 rewrites this part altogether.
pkg/sql/logictest/testdata/logic_test/bytes, line 122 at r1 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
make this a subtest and drop the table at the end of it
Done.
pkg/sql/distsqlrun/api.proto
Outdated
optional BytesEncodeFormat bytes_encode_format = 10 [(gogoproto.nullable) = false]; | ||
} | ||
|
||
// BytesEncodeFormat is the configurationf or bytes to string conversions. |
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.
typo
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.
Good catch! Fixed.
It was not propagated previously, causing distributed queries to improperly convert byte arrays to strings. Release note (bug fix): the parameter `bytea_output` is now properly effective for distributed queries.
c621d25
to
f21f3c6
Compare
bors r+ |
27951: sql: propagate bytea_output to distsql processors r=knz a=knz Fixes #27950. It was not propagated previously, causing distributed queries to improperly convert byte arrays to strings. Release note (bug fix): the parameter `bytea_output` is now properly effective for distributed queries. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Build succeeded |
27952: sql: complete the implementation of extra_float_digits r=knz a=knz First commit from #27951. The code previously recognized the name of the session variable but completely ignored the value that was specified. This would cause silently invalid results to be processed in conversions from float to string, and when float values are returned over pgwire. This patch fixes the situation by making the setting work as intended by clients. Release note (sql change): The support for `extra_float_digits` is extended for compatibility with PostgreSQL. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Fixes #27950.
It was not propagated previously, causing distributed queries to
improperly convert byte arrays to strings.
Release note (bug fix): the parameter
bytea_output
is now properlyeffective for distributed queries.