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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your life would be simpler if you changed the Query class to be backed by Protos. Inspiration: https://github.com/googleapis/java-firestore/blob/9ff2f41b765c8878c3b3fb7df962f6f1ed537f05/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java#L234
33a6f53
to
b9ad354
Compare
…e source of truth
There was a problem hiding this 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.
a77503f
to
10d797c
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
…d-go into query-serialization
There was a problem hiding this 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!
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.