Skip to content

Comments

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

Merged
stephenplusplus merged 4 commits intogoogleapis:masterfrom
stephenplusplus:pr-2417
Jul 5, 2017
Merged

spanner: Properly handle multi-row mutations with heterogeneous columns#2426
stephenplusplus merged 4 commits intogoogleapis:masterfrom
stephenplusplus:pr-2417

Conversation

@stephenplusplus
Copy link
Contributor

@stephenplusplus stephenplusplus commented Jun 30, 2017

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

googlebot commented Jun 30, 2017

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

coveralls commented Jun 30, 2017

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

coveralls commented Jun 30, 2017

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

coveralls commented Jul 1, 2017

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
sofisl pushed a commit that referenced this pull request Feb 24, 2026
@dconeybe
change: pool.ts: add even more logging (#2426)
ef34d38
sofisl pushed a commit that referenced this pull request Feb 25, 2026
@release-please
chore(main): release 7.9.0 (#2426)
24cfaa1 
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@vkedia vkedia vkedia left review comments

@callmehiphop callmehiphop callmehiphop approved these changes

Reviewers whose approvals may not affect merge requirements

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