Firestore: use Integer.compare() and Long.compare() instead Firestore bespoke implementations #7109
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… bespoke `Util.compareIntegers()` method. The `Util.compareIntegers()` method was originally added because it required minSdkVersion of 19 or greater which, at the time, was not the case. However, minSdkVersion has been 21 for some time now (and is increasing to 23 in the foreseeable future) so there is no longer any need for the bespoke implementation.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request effectively cleans up the codebase by replacing the custom Util.compareIntegers() with the standard Integer.compare(), leveraging the updated minSdkVersion. The changes are well-executed, and further improvements include updating the CHANGELOG.md placeholder and using Character.compare() for char comparisons in Util.java for better type safety.
firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java
Outdated
Show resolved
Hide resolved
Integer.compare() instead of Util.compareIntegers()
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/gemini review /gemini summary |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes integer and long comparisons in the Firestore Android SDK by replacing bespoke Util.compareIntegers() and Util.compareLongs() with standard Java Integer.compare() and Long.compare() methods, updates related imports, and cleans up the now-redundant utility methods. It also amends the UTF-8 string comparison tests to validate comparisons based on sign rather than exact values.
- Replaced all calls to
Util.compareIntegers()/compareLongs()withInteger.compare(),Long.compare(), orCharacter.compare(). - Removed redundant
compareIntegersandcompareLongsmethods fromUtilandNumberComparisonHelper. - Updated
UtilTestto assert viaInteger.signum()and introduced asignNamehelper. - Updated
CHANGELOG.mdto document the change.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java | Removed custom compare methods and replaced usages with standard APIs. |
| firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/NumberComparisonHelper.java | Removed compareLongs, replaced calls with Long.compare(). |
| firebase-firestore/src/main/java/com/google/firebase/firestore/model/*.java | Updated multiple model comparators to use Integer.compare()/Long.compare(). |
| firebase-firestore/src/main/java/com/google/firebase/firestore/local/*.java | Updated local sorting comparators and removed unused imports. |
| firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java | Changed test assertions to compare signum and added signName. |
| firebase-firestore/CHANGELOG.md | Added entry for using Integer.compare() and Long.compare(). |
Comments suppressed due to low confidence (1)
firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java:125
- The error message for the expected value prints only the sign description but omits the actual expected numeric value. Consider adding
(" + expected + ")aftersignName(expected)so the message shows both the sign and the numeric value.
+ signName(expected)
dconeybe
added a commit
to googleapis/java-firestore
that referenced
this pull request
Jul 7, 2025
The semantics of this logic were originally fixed by #1967, but this fix caused a material performance degradation, which was then improved by #2021. The performance was, however, still suboptimal, and this PR further improves the speed back to close to its original speed and, serendipitously, simplifies the algorithm too. This commit effectively ports the following two PRs from the firebase-android-sdk repository: - firebase/firebase-android-sdk#7098 - firebase/firebase-android-sdk#7109
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Notes
Startup Times
|
tom-andersen
approved these changes
Jul 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces the bespoke
Util.compareIntegers()andUtil.compareLongs()methods with standard JavaInteger.compare(),Long.compare()andCharacter.compare()methods. It also updates the tests to validate comparisons based on sign rather than exact values, as these tests were previously brittle, erroneously failing due toCharacter.compare()returning values other than1,0, and-1.The
Util.compareIntegers()andUtil.compareLongs()methods were originally added because they required minSdkVersion of 19 or greater which, at the time, was not the case. However, minSdkVersion has been 21 for some time now (and is increasing to 23 in the foreseeable future) so there is no longer any need for the bespoke implementations.Highlights
Util.compareIntegers()andUtil.compareLongs()withInteger.compare(),Long.compare(), orCharacter.compare(), as appropriate.compareIntegersandcompareLongsmethods fromUtilandNumberComparisonHelper.UtilTestto assert viaInteger.signum()instead of testing exact equality with1,0, or-1.