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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Jul 5, 2017
Merged

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

merged 4 commits into from
Jul 5, 2017

Conversation

stephenplusplus
Copy link
Contributor

Fixes #2411

This adds a commit with tests on top of the work done by @jscinoz in #2417.

jscinoz and others added 2 commits June 30, 2017 15:52
@jscinoz @stephenplusplus
spanner: Properly handle multi-row mutations with heterogeneous columns
1e58fd6 
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.
@stephenplusplus
refactor + unit tests + system test
d3f1d5d
@stephenplusplus stephenplusplus added the api: spanner Issues related to the Spanner API. label Jun 30, 2017
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jun 30, 2017
@stephenplusplus stephenplusplus mentioned this pull request Jun 30, 2017
Closed
@stephenplusplus stephenplusplus mentioned this pull request Jun 30, 2017
Closed
@coveralls
Copy link

Coverage Status

Coverage increased (+6.0e-06%) to 99.988% when pulling d3f1d5d on stephenplusplus:pr-2417 into c9037b6 on GoogleCloudPlatform:master.

vkedia
vkedia reviewed Jun 30, 2017
View reviewed changes
packages/spanner/system-test/spanner.js Outdated
BoolValue: true,
IntValue: 10
}
], done);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@coveralls
Copy link

Coverage Status

Coverage increased (+6.0e-06%) to 99.988% when pulling 3ae6621 on stephenplusplus:pr-2417 into c9037b6 on GoogleCloudPlatform:master.

@vkedia
Copy link
Contributor

vkedia commented Jun 30, 2017

What would happen if the two rows have different number of columns? I think you should error out in that case.

stephenplusplus
stephenplusplus commented Jul 1, 2017
View reviewed changes
packages/spanner/src/transaction-request.js
var missingColumns = columns.filter(column => keys.indexOf(column) === -1);

if (missingColumns.length > 0) {
throw new Error([

This comment was marked as spam.

Sign in to view

@coveralls
Copy link

Coverage Status

Coverage increased (+1.0e-05%) to 99.988% when pulling 810465f on stephenplusplus:pr-2417 into c9037b6 on GoogleCloudPlatform:master.

@stephenplusplus stephenplusplus merged commit d395a4b into googleapis:master Jul 5, 2017
This was referenced Jul 5, 2017
Merged
Merged
@stephenplusplus stephenplusplus mentioned this pull request Dec 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vkedia vkedia vkedia left review comments

@callmehiphop callmehiphop callmehiphop approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the Spanner API. cla: no This human has *not* signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stephenplusplus @googlebot @coveralls @vkedia @callmehiphop @jscinoz