KEMBAR78
Fix: sort document reference by long type id by milaGGL · Pull Request #1945 · googleapis/java-firestore · GitHub
Skip to content

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented Dec 5, 2024

Document ID supports strings and well as long integers in the format of "__id<Long>__" . When sorting documents by document ID, it should be sorted in the following order:

  1. Long (numeric order)
  2. String (lexicographic order)

@milaGGL milaGGL requested a review from a team as a code owner December 5, 2024 17:18
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/java-firestore API. labels Dec 5, 2024
@cloud-java-bot cloud-java-bot requested a review from a team as a code owner December 5, 2024 17:20
@milaGGL milaGGL changed the title Mila/fix sdk sort document reference bug Fix: sort document reference by long type id Dec 5, 2024
Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

LGTM. A minor improvement suggested below

@ehsannas ehsannas assigned milaGGL and unassigned ehsannas Dec 6, 2024
@milaGGL milaGGL requested a review from TomasMorton December 6, 2024 22:07
@ehsannas
Copy link
Contributor

ehsannas commented Dec 6, 2024

We should add a testcase for negative numeric ID if that's allowed.

@milaGGL milaGGL added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 9, 2024
@yoshi-kokoro yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 9, 2024
@milaGGL milaGGL merged commit c38e3ad into main Dec 17, 2024
23 checks passed
@milaGGL milaGGL deleted the mila/fix-sdk-sort-document-reference-bug branch December 17, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the googleapis/java-firestore API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants