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(firestore): Support DocumentRefs in OrderBy, Add Query.Serialize, Query.Deserialize for cross machine serialization #4347

Merged
merged 44 commits into from Sep 2, 2021

Conversation

crwilcox
Copy link
Contributor

@crwilcox crwilcox commented Jun 29, 2021

Changes to the internal workings of query to store things in a proto representation. I wanted to do this for start/end cursors, but due to the allowing of order to happen after startat, it isn't feasible. There is a bug #4448 to track making a breaking change that would allow simplifying the cursors. See d4f34b0 for that approach (it is untidy, but should be functional)

This also enables serialization to proto which should make it possible for users to stream the query on another machine/process.

Also extended orderby to support document refs, a long todo that needed to be implemented to support round tripping.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 29, 2021
Copy link

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

firestore/integration_test.go Outdated Show resolved Hide resolved
firestore/query.go Outdated Show resolved Hide resolved
firestore/query.go Outdated Show resolved Hide resolved
firestore/query.go Outdated Show resolved Hide resolved
Copy link

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

Basically LGTM, but I do think you should also convert the Cursor to Proto which should simplify the new code.

firestore/integration_test.go Show resolved Hide resolved
firestore/query.go Outdated Show resolved Hide resolved
@crwilcox crwilcox marked this pull request as ready for review July 15, 2021 21:50
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Seems basically fine, just trying to understand better what this is for.

firestore/client.go Outdated Show resolved Hide resolved
firestore/integration_test.go Outdated Show resolved Hide resolved
firestore/query.go Outdated Show resolved Hide resolved
firestore/query.go Outdated Show resolved Hide resolved
firestore/query.go Show resolved Hide resolved
firestore/query.go Show resolved Hide resolved
firestore/query.go Outdated Show resolved Hide resolved
firestore/client.go Outdated Show resolved Hide resolved
firestore/collgroupref.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Nice work on this. I think we could still use a longer worked example and explanation for how to use this (in Go and across languages) but that can come later.

@crwilcox crwilcox changed the title feat(firestore): Allow deserializing proto to Query feat(firestore): Support DocumentRefs in order, Add Query.ToProto, Query.FromProto for cross machine serialization Sep 1, 2021
@crwilcox crwilcox changed the title feat(firestore): Support DocumentRefs in order, Add Query.ToProto, Query.FromProto for cross machine serialization feat(firestore): Support DocumentRefs in OrderBy, Add Query.ToProto, Query.FromProto for cross machine serialization Sep 1, 2021
@crwilcox crwilcox changed the title feat(firestore): Support DocumentRefs in OrderBy, Add Query.ToProto, Query.FromProto for cross machine serialization feat(firestore): Support DocumentRefs in OrderBy, Add Query.Serialize, Query.Deserialize for cross machine serialization Sep 1, 2021
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Nice, I like this new surface!

@crwilcox crwilcox merged commit a0f7a02 into googleapis:master Sep 2, 2021
@crwilcox crwilcox deleted the query-serialization branch September 2, 2021 16:08
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.

None yet

4 participants