Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

refactor!: expand SqlParams members into PartitionQueryParams #1263

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

devjgm
Copy link
Contributor

@devjgm devjgm commented Feb 13, 2020

PartitionQueryParams corresponds to PartitionQueryRequest
(https://github.com/googleapis/googleapis/blob/2c17ac33b226194041155bb5340c3f34733f1b3a/google/spanner/v1/spanner.proto#L672),
and SqlParams corresponds to ExecuteSqlRequest
(https://github.com/googleapis/googleapis/blob/2c17ac33b226194041155bb5340c3f34733f1b3a/google/spanner/v1/spanner.proto#L439).
The former does not "include" the latter, so there may be options or
fields in SqlParams that don't belong in PartitionQueryParams.
Therefore, we should break this relationship now, while we can.


This change is Reviewable

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 13, 2020
@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #1263 into master will decrease coverage by 0.02%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1263      +/-   ##
==========================================
- Coverage   94.84%   94.82%   -0.03%     
==========================================
  Files         184      184              
  Lines       13805    13806       +1     
==========================================
- Hits        13094    13091       -3     
- Misses        711      715       +4
Impacted Files Coverage Δ
google/cloud/spanner/connection.h 100% <ø> (ø) ⬆️
google/cloud/spanner/client.cc 97.01% <100%> (-0.75%) ⬇️
google/cloud/spanner/internal/connection_impl.cc 96.24% <100%> (-0.01%) ⬇️
...gle/cloud/spanner/internal/connection_impl_test.cc 97.86% <50%> (-0.01%) ⬇️
...on_tests/rpc_failure_threshold_integration_test.cc 85.71% <0%> (-2.07%) ⬇️
...ud/spanner/integration_tests/client_stress_test.cc 82.45% <0%> (-0.88%) ⬇️
google/cloud/spanner/keys.h 100% <0%> (ø) ⬆️
...anner/integration_tests/client_integration_test.cc 98.48% <0%> (ø) ⬆️
google/cloud/spanner/value.h 92.9% <0%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44d687f...daa742b. Read the comment docs.

PartitionQueryParams corresponds to PartitionQueryRequest
(https://github.com/googleapis/googleapis/blob/2c17ac33b226194041155bb5340c3f34733f1b3a/google/spanner/v1/spanner.proto#L672),
and SqlParams corresponds to ExecuteSqlRequest
(https://github.com/googleapis/googleapis/blob/2c17ac33b226194041155bb5340c3f34733f1b3a/google/spanner/v1/spanner.proto#L439).
The former does not "include" the latter, so there may be options or
fields in SqlParams that don't belong in PartitionQueryParams.
Therefore, we should break this relationship now, while we can.
Copy link
Member

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @scotthart)

@devjgm devjgm merged commit e90e64e into googleapis:master Feb 13, 2020
@devjgm devjgm deleted the split-sql-partition branch February 13, 2020 14:20
devjgm added a commit to devjgm/google-cloud-cpp that referenced this pull request May 7, 2020
…apis/google-cloud-cpp-spanner#1263)

PartitionQueryParams corresponds to PartitionQueryRequest
(https://github.com/googleapis/googleapis/blob/2c17ac33b226194041155bb5340c3f34733f1b3a/google/spanner/v1/spanner.proto#L672),
and SqlParams corresponds to ExecuteSqlRequest
(https://github.com/googleapis/googleapis/blob/2c17ac33b226194041155bb5340c3f34733f1b3a/google/spanner/v1/spanner.proto#L439).
The former does not "include" the latter, so there may be options or
fields in SqlParams that don't belong in PartitionQueryParams.
Therefore, we should break this relationship now, while we can.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants