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

fix: support INSERT from SELECT clause with args #306

Merged
merged 3 commits into from Apr 15, 2021

Conversation

IlyaFaer
Copy link
Contributor

Spanner supports doing INSERT with values takes from a SELECT clause. However, DB API is processing such a cases incorrectly, so such a query, for example:

INSERT INTO manual_pk (id, data)
SELECT manual_pk.id + %s AS anon_1, manual_pk.data
FROM manual_pk
WHERE manual_pk.data IN (%s, %s)

Will fail with a syntax error:

400 Syntax error: Illegal input character \"%\" [at 3:23]\nSELECT manual_pk.id + %s AS anon_1, manual_pk.data\n

Fixing this bug.

@IlyaFaer IlyaFaer added api: spanner Issues related to the googleapis/python-spanner API. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Apr 12, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 12, 2021
@@ -276,7 +276,7 @@ def parse_insert(insert_sql, params):
if not after_values_sql:
# Case b)
insert_sql = sanitize_literals_for_upload(insert_sql)
return {"sql_params_list": [(insert_sql, None)]}
return {"sql_params_list": [(insert_sql, params)]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the previous author of the DB API didn't notice that INSERT from SELECT will be a case b), and it can include some args. Thus, passing None instead of args is not correct.

@IlyaFaer IlyaFaer marked this pull request as ready for review April 12, 2021 08:42
@IlyaFaer IlyaFaer requested a review from a team as a code owner April 12, 2021 08:42
@IlyaFaer IlyaFaer requested a review from larkee April 12, 2021 08:42
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

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

LGTM. Does the docstring need updating to reflect this change?

@larkee larkee merged commit 0dcda5e into googleapis:master Apr 15, 2021
@IlyaFaer IlyaFaer deleted the insert_from_select branch April 15, 2021 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner API. cla: yes This human has signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants