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.

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

feat: Add support to convert a Query to a BundledQuery. #1034

Merged
merged 10 commits into from
Apr 22, 2020

Conversation

wu-hui
Copy link
Contributor

@wu-hui wu-hui commented Apr 21, 2020

Note the tricky thing here is: we introduced limitToLast, which means we need to decide if the structuredQuery saved in the bundle should be translated or not.

This implementation saves it without translation, which means the client SDK will use the structureQuery and limitType saved here to construct a new client Query, and leaves the translation to the existing logic on the client side.

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

codecov bot commented Apr 21, 2020

Codecov Report

Merging #1034 into wuandy/Bundles will decrease coverage by 0.00%.
The diff coverage is 97.22%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           wuandy/Bundles    #1034      +/-   ##
==================================================
- Coverage           97.46%   97.45%   -0.01%     
==================================================
  Files                  25       25              
  Lines               16015    16044      +29     
  Branches             1218     1217       -1     
==================================================
+ Hits                15609    15636      +27     
- Misses                403      405       +2     
  Partials                3        3              
Impacted Files Coverage Δ
dev/src/reference.ts 99.73% <97.22%> (-0.08%) ⬇️

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 4dac6d1...2941d76. Read the comment docs.

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

This LGTM, minus some nits. Thanks!

structuredQuery,
};

reqOpts.transaction = transactionId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Merge this into the assignment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


reqOpts.transaction = transactionId;

return reqOpts;
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible inline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


/**
* Internal method for serializing a query to its RunQuery proto
* representation with an optional transaction id.
Copy link
Contributor

Choose a reason for hiding this comment

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

This documentation seems not applicable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

},
],
};
if (this._queryOptions.allDescendants) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Empty line above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

o.toProto()
);
}
structuredQuery.startAt = this.toCursor(this._queryOptions.startAt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Empty line above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Thanks!

@wu-hui wu-hui changed the base branch from wuandy/BundlesProto to wuandy/Bundles April 22, 2020 12:48
@wu-hui wu-hui changed the base branch from wuandy/Bundles to wuandy/BundlesProto April 22, 2020 12:50
@wu-hui wu-hui changed the base branch from wuandy/BundlesProto to wuandy/Bundles April 22, 2020 12:51
@wu-hui wu-hui changed the base branch from wuandy/Bundles to wuandy/BundlesProto April 22, 2020 12:53
@wu-hui wu-hui changed the base branch from wuandy/BundlesProto to wuandy/Bundles April 22, 2020 12:53
@wu-hui wu-hui merged commit 5978e72 into wuandy/Bundles Apr 22, 2020
wu-hui added a commit that referenced this pull request Apr 24, 2020
* Update from googleapis.git

* Add bundle.proto.

* Move bundle.proto to a different folder.

* Add support to convert a Query to a BundledQuery.

* Move the proto file again

* Move the proto file again

* Update package name.

* Update to use new package name.
wu-hui added a commit that referenced this pull request Apr 24, 2020
* Update from googleapis.git

* Add bundle.proto.

* Move bundle.proto to a different folder.

* Add support to convert a Query to a BundledQuery.

* Move the proto file again

* Move the proto file again

* Update package name.

* Update to use new package name.
wu-hui added a commit that referenced this pull request May 13, 2020
* Update from googleapis.git

* Add bundle.proto.

* Move bundle.proto to a different folder.

* Add support to convert a Query to a BundledQuery.

* Move the proto file again

* Move the proto file again

* Update package name.

* Update to use new package name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants