sql: switch QueryArguments, PlaceholderTypes to slices #33716
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Release note: None
|
This change is |
RaduBerinde
force-pushed
the
placeholder-slices
branch
2 times, most recently
from
075c166 to
e326962
Compare
We switch `PlaceholderTypes` to a slice instead of a map. We also fix the handling of cases where some placeholders are unused (e.g. `SELECT $2:::int`) which now error out (before they would crash during execution). Note that PG also errors out in this case. Fixes cockroachdb#30086. Release note (bug fix): Preparing queries with missing placeholders (e.g. `SELECT $2::int`) now results in an error.
RaduBerinde
force-pushed
the
placeholder-slices
branch
from
e326962 to
651f11b
Compare
jordanlewis
approved these changes
Jan 15, 2019
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained
|
bors r+ |
Build failed (retrying...) |
craig bot
pushed a commit
that referenced
this pull request
Jan 16, 2019
33716: sql: switch QueryArguments, PlaceholderTypes to slices r=RaduBerinde a=RaduBerinde #### sql: change QueryArguments map to a slice Release note: None #### sql: switch PlaceholderTypes to a slice We switch `PlaceholderTypes` to a slice instead of a map. We also fix the handling of cases where some placeholders are unused (e.g. `SELECT $2:::int`) which now error out (before they would crash during execution). Note that PG also errors out in this case. Fixes #30086. Release note (bug fix): Preparing queries with missing placeholders (e.g. `SELECT $2::int`) now results in an error. Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Build succeeded |
| @@ -440,9 +440,6 @@ INSERT INTO add_default (a) VALUES (3) | |||
| statement error could not parse "foo" as type int | |||
| ALTER TABLE add_default ALTER COLUMN b SET DEFAULT 'foo' | |||
|
|
|||
| statement error variable sub-expressions are not allowed in DEFAULT | |||
| ALTER TABLE add_default ALTER COLUMN b SET DEFAULT $1 | |||
There was a problem hiding this comment.
I think they should be converted to prepare a(int) as alter table ...
Otherwise a naked $1 is not valid in a logic test.
There was a problem hiding this comment.
This was meant to test the "variable sub-expressions" error for alter table. Using placeholders without prepare is not valid, and now we get a different (earlier) error that's not specific to alter table. There are other testcases for the same error below though.
There was a problem hiding this comment.
👍 thanks for the explanation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
sql: change QueryArguments map to a slice
Release note: None
sql: switch PlaceholderTypes to a slice
We switch
PlaceholderTypesto a slice instead of a map.We also fix the handling of cases where some placeholders are unused
(e.g.
SELECT $2:::int) which now error out (before they would crashduring execution). Note that PG also errors out in this case.
Fixes #30086.
Release note (bug fix): Preparing queries with missing placeholders
(e.g.
SELECT $2::int) now results in an error.