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

spanner: Properly handle multi-row mutations with heterogeneous columns #2417

Closed
wants to merge 1 commit into from

Conversation

jscinoz
Copy link

@jscinoz jscinoz commented Jun 28, 2017

Prior to this change, attempting to insert multiple rows at once with Table#insert or
Transaction#insert where some rows lack a value for nullable columns would result in data attempting
to be inserted into the incorrect column, due to a flawed assumption in transaction-request.js that
every single row contains exact same set of columns, and that the order of keys returned by
Object.keys would be identical for each and every row.

This fixes #2411.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 28, 2017
Prior to this change, attempting to insert multiple rows at once with Table#insert or
Transaction#insert where some rows lack a value for nullable columns would result in data attempting
to be inserted into the incorrect column, due to a flawed assumption in transaction-request.js that
every single row contains exact same set of columns, and that the order of keys returned by
Object.keys would be identical for each and every row.

This fixes googleapis#2411.
@jscinoz jscinoz force-pushed the feature/fix-2411 branch from 0b77c86 to a2a8ce5 Compare June 28, 2017 01:20
@coveralls
Copy link

Coverage Status

Coverage increased (+1.0e-05%) to 99.988% when pulling a2a8ce5 on AvokaTech:feature/fix-2411 into ca0d96c on GoogleCloudPlatform:master.

@stephenplusplus
Copy link
Contributor

Thank you very much! @callmehiphop and I are in the middle of a few other tasks for the week, but we will check this out as soon as possible. 👍

@stephenplusplus stephenplusplus added the api: spanner Issues related to the Spanner API. label Jun 28, 2017
@vkedia
Copy link
Contributor

vkedia commented Jun 28, 2017

@jscinoz Thanks for sending this. @stephenplusplus @lukesneeringer Can we please prioritise this. Anything that can cause incorrect data to be stored in Spanner makes me really nervous.

@stephenplusplus
Copy link
Contributor

I made a few changes, but didn't have permission to push to the PR branch. I've continued this PR in #2426.

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 Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
5 participants