Rewrite PartialResultStream #208

stephenplusplus · 2018-05-29T17:00:00Z

Fixes #180 and #198 by reworking the way we process streamed rows from PartialResultStream on Spanner. The high-level difference is this

use RowBuilder for all streams. We had two code paths previously
do not block on returning until we have a non-chunked row. We can hold the final value of a chunked segment and begin returning other data
handle partial segments. Occasionally we will get a partial row and shouldn't try to send that to the user.

To Dos

Unit tests

…tries

…wice safely

…as we have a row.

googlebot · 2018-05-29T17:00:08Z

☹️ Sorry, but only Googlers may change the label cla: yes.

stephenplusplus · 2018-05-29T18:13:34Z

@crwilcox I made some changes, but my tests still pass. If this looks okay to you, I'll move onto unit tests.

codecov · 2018-05-29T18:16:17Z

Codecov Report

Merging #208 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #208   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        1402   1385   -17     
=====================================
- Hits         1402   1385   -17

Impacted Files	Coverage Δ
src/codec.js	`100% <100%> (ø)`	⬆️
src/partial-result-stream.js	`100% <100%> (ø)`	⬆️
src/row-builder.js	`100% <100%> (ø)`	⬆️

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 10a99c0...a69c500. Read the comment docs.

crwilcox · 2018-05-29T18:21:17Z

Thanks for stripping out that dead code. I had forgotten about that. I will stress it over lunch here just to make sure but it looks good after 5 loops. Feel free to move forward.

src/partial-result-stream.js

-    var field = fields[index];
-    return {
-      name: field.name,
-      value: codec.decode(value, field),


stephenplusplus · 2018-05-31T21:40:55Z

Status update:

I took over the PR and did some refactors. Wrote unit tests, all was well. One quick check that the system tests work, then 💥 -- I don't remember the exact number, but let's say ~20 tests failed. I reverted my refactors to see if it was something I did, but unfortunately, same errors.

I realized something we didn't carry over when @crwilcox made his changes was a call to codec.decode. Since things were re-arranged from the old way, it wasn't exactly clear where it should go. But, I put it back, and all but one test fails. And I can't figure that one out.

Basically, I'm hoping @crwilcox can dive back in for a second to see if he has any ideas. I'll leave some comments on this PR next to the behavioral changes I've made since you last touched the code.

src/row-builder.js

@@ -72,7 +71,7 @@ RowBuilder.formatValue = function(field, value) {
  }

  if (field.code !== 'STRUCT') {
-    return value;
+    return codec.decode(value, field);


src/codec.js

@@ -118,7 +118,7 @@ codec.generateToJSONFromRow = generateToJSONFromRow;
 * @private
 */
 function decode(value, field) {
-  function decodeValue_(decoded, type) {
+  function decodeValue_(decoded, type, options) {


src/row-builder.js

@@ -46,7 +45,7 @@ RowBuilder.getValue = function(obj) {
  var value = obj;

  if (obj && obj.kind) {
-    value = obj[obj.kind];
+    value = commonGrpc.Service.decodeValue_(obj);


src/row-builder.js


+  if (!is.empty(this.rows[0]) && this.currentRow.length !== this.fields.length) {


stephenplusplus · 2018-06-04T16:27:12Z

@crwilcox I think we're finally good again. Units, systems, and your revision of the "issue-180.js" file I've checked are working. It would be great if you could confirm!

crwilcox · 2018-06-04T16:46:09Z

Some system tests are failing locally after the edits:

 insert & query
        3) should query in callback mode
        4) should query in promise mode
        5) should query in stream mode

Coverage numbers:

stephenplusplus · 2018-06-04T16:47:23Z

100% line coverage is what we go for. Can you show more details about the system test errors?

crwilcox · 2018-06-04T18:11:17Z

Sure, but the system tests seem to also be an issue on builds before your commits.

3) Spanner
       Tables
         insert & query
           should query in callback mode:

      Uncaught AssertionError [ERR_ASSERTION]: { SingerId: 'gcloud-tests-id-1278e710',
  Name: 'gcloud-tests-name-1278e711',
  Float: null,
  Int: null,
  Info: null,
  Create deepEqual { SingerId: 'gcloud-tests-id-e12752a0',
  Name: 'gcloud-tests-name-e12752a1',
  Float: 8.2,
  Int: 2,
  Info: <Buffer 67 63 6c 6
      + expected - actual

       {
      -  "Accents": [null]
      -  "Created": [null]
      -  "DOB": [null]
      -  "Float": [null]
      -  "HasGear": [null]
      -  "Info": [null]
      -  "Int": [null]
      -  "Name": "gcloud-tests-name-1278e711"
      -  "PhoneNumbers": [null]
      -  "SingerId": "gcloud-tests-id-1278e710"
      +  "Accents": [
      +    "jamaican"
      +  ]
      +  "Created": [Date: 2018-06-04T16:46:51.082Z]
      +  "DOB": [Date: 1969-08-20T00:00:00.000Z]
      +  "Float": 8.2
      +  "HasGear": true
      +  "Info": [Buffer: [
      +    103
      +    99
      +    108
      +    111
      +    117
      +    100

stephenplusplus · 2018-06-04T19:09:32Z

@alexander-fenster ran the system tests and had a different failure. I don't believe these are related to our PR here. @callmehiphop can you run these?

I suppose we could always do a merge to let Circle have a go at them, then revert it if necessary.

crwilcox · 2018-06-04T20:43:16Z

Reran the tests and they pass on master and on this branch. I think this is safe to merge.

alexander-fenster

LGTM, hope I correctly understood the idea of the changes.

crwilcox added 12 commits May 23, 2018 13:24

add check for missing resumeToken and chunk if this occurs

0ecb17e

add additional check around data. only chunk on missing if partial en…

728cde0

…tries

adjusting how we detect partial rows

ca97b4f

rowbuilder should only add rows once to output. build can be called t…

b656747

…wice safely

chunked rows shouldn't automatically be json. Process chunks as soon …

65fcd9b

…as we have a row.

typo s/field/fields

9dff137

for-stephen-busted-technically

725c60b

store work on rewrite

07d2831

store work on rewrite

b511170

commit local changes

266c25c

cleanup code

e936491

combine toJSON and toJSONnobuild

2704d06

stephenplusplus added the do not merge label May 29, 2018

ghost assigned stephenplusplus May 29, 2018

ghost added the cla: yes label May 29, 2018

googlebot removed the cla: yes label May 29, 2018

googlebot added the cla: yes label May 29, 2018

Quick refactor

ed776be

prettier, lint, and reomve lodash.chunk

58ce7ae

stephenplusplus added 2 commits May 30, 2018 14:51

Test PartialResultStream

b1dbdde

Decoding row value confusion

9210fc9