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

feat: streaming Value STRING only quotes in aggregates #1401

Merged
merged 2 commits into from
Mar 17, 2020

Conversation

devjgm
Copy link
Contributor

@devjgm devjgm commented Mar 17, 2020

Fixes: #1391

This PR changes the way Value(string) streams its values.
Specifically, double quotes are no longer included when streaming scalar
Value(string) values. For example: std::cout << Value("foo") would
output foo (no quotes). This behavior matches that of std::cout << std::string("foo").

However, when streaming strings as part of an aggregate type
(std::vector or std::tuple), strings will be wrapped in double quotes
so that the contents of the string does not break the
structure/formatting of the printed array/struct itself. Any double
quotes in the string itself will be backslash escaped.

This PR also adds double-quoting to field names in tuples, which are
strings and should therefore also be quoted like other string fields in
aggregates.


This change is Reviewable

Fixes: googleapis#1391

This PR changes the way `Value(string)` streams its values.
Specifically, double quotes are no longer included when streaming scalar
`Value(string)` values. For example: `std::cout << Value("foo")` would
output `foo` (no quotes). This behavior matches that of `std::cout <<
std::string("foo")`.

However, when streaming strings as part of an aggregate type
(std::vector or std::tuple), strings *will be* wrapped in double quotes
so that the contents of the string does not break the
structure/formatting of the printed array/struct itself. Any double
quotes in the string itself will be backslash escaped.

This PR also adds double-quoting to field names in tuples, which are
strings and should therefore also be quoted like other string fields in
aggregates.
@devjgm devjgm requested review from coryan and devbww March 17, 2020 14:41
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 17, 2020
@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #1401 into master will decrease coverage by 0.02%.
The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1401      +/-   ##
==========================================
- Coverage   94.38%   94.35%   -0.03%     
==========================================
  Files         190      191       +1     
  Lines       15659    15711      +52     
==========================================
+ Hits        14779    14824      +45     
- Misses        880      887       +7
Impacted Files Coverage Δ
google/cloud/spanner/sql_statement_test.cc 100% <100%> (ø) ⬆️
google/cloud/spanner/value_test.cc 99.33% <100%> (+0.01%) ⬆️
google/cloud/spanner/value.cc 96.76% <95%> (-0.25%) ⬇️
...loud/spanner/internal/partial_result_set_resume.cc 90.9% <0%> (-4.55%) ⬇️
google/cloud/spanner/results.h 95.83% <0%> (-4.17%) ⬇️
google/cloud/spanner/internal/polling_loop.h 91.89% <0%> (-2.71%) ⬇️
...ud/spanner/integration_tests/client_stress_test.cc 80.55% <0%> (-1.86%) ⬇️
...loud/spanner/internal/partial_result_set_source.cc 93.24% <0%> (-1.36%) ⬇️
google/cloud/spanner/samples/samples.cc 89.38% <0%> (-0.25%) ⬇️
google/cloud/spanner/internal/session.h 100% <0%> (ø) ⬆️
... and 10 more

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 00ca369...6d7775d. Read the comment docs.

Copy link
Contributor

@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.

The code looks good, one suggestion for you to consider. If it does not work I am Okay with merging as-is.

Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @devbww and @devjgm)


google/cloud/spanner/value.cc, line 98 at r1 (raw file):

std::ostream& EscapeQuotes(std::ostream& os, std::string const& s) {
  for (auto const& c : s) {
    if (c == '"') os << "\\";

Hmm... Is the goal to make the output usable as a string literal in SQL? Then maybe using triple quotes as a delimiter would be simpler?

https://cloud.google.com/spanner/docs/lexical#string_and_bytes_literals

@devjgm devjgm marked this pull request as ready for review March 17, 2020 15:18
Copy link
Contributor Author

@devjgm devjgm left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @coryan and @devbww)


google/cloud/spanner/value.cc, line 98 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Hmm... Is the goal to make the output usable as a string literal in SQL? Then maybe using triple quotes as a delimiter would be simpler?

https://cloud.google.com/spanner/docs/lexical#string_and_bytes_literals

Producing Spanner string literals is explicitly not a goal.

Even if we wrapped strings in triple quotes, I believe we'd still need to escape triple quotes that appeared in the string data, so I'm not sure if it'd be simpler.

@devjgm devjgm merged commit 4099652 into googleapis:master Mar 17, 2020
@devjgm devjgm deleted the value-string branch March 17, 2020 16:50
devjgm added a commit to devjgm/google-cloud-cpp that referenced this pull request May 7, 2020
…ogle-cloud-cpp-spanner#1401)

* feat: streaming Value STRING only quotes in aggregates

Fixes: googleapis/google-cloud-cpp-spanner#1391

This PR changes the way `Value(string)` streams its values.
Specifically, double quotes are no longer included when streaming scalar
`Value(string)` values. For example: `std::cout << Value("foo")` would
output `foo` (no quotes). This behavior matches that of `std::cout <<
std::string("foo")`.

However, when streaming strings as part of an aggregate type
(std::vector or std::tuple), strings *will be* wrapped in double quotes
so that the contents of the string does not break the
structure/formatting of the printed array/struct itself. Any double
quotes in the string itself will be backslash escaped.

This PR also adds double-quoting to field names in tuples, which are
strings and should therefore also be quoted like other string fields in
aggregates.

* quiet switch fallthrough warning
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.

Consider: Should Value::op<< STRING be wrapped in quotes?
3 participants