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
Member

@IlyaFaer IlyaFaer commented Apr 12, 2021

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 type: bug labels Apr 12, 2021
@google-cla google-cla bot added the cla: yes 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
Member Author

@IlyaFaer IlyaFaer Apr 12, 2021

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 Apr 12, 2021
@IlyaFaer IlyaFaer requested a review from as a code owner Apr 12, 2021
@IlyaFaer IlyaFaer requested a review from larkee Apr 12, 2021
larkee
larkee approved these changes Apr 14, 2021
Copy link
Contributor

@larkee larkee left a comment

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

@larkee larkee merged commit 0dcda5e into googleapis:master Apr 15, 2021
8 checks passed
@IlyaFaer IlyaFaer deleted the insert_from_select branch Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants