Skip to content

Fix: Use lazy encoding for utf-8 encoded string comparison #2299

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

Merged
merged 7 commits into from
Mar 3, 2025

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented Feb 18, 2025

The previous fix created a performance issue due to expensive UTF-8 encoding. Update compareUtf8Strings to use lazy encoding instead.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Feb 18, 2025
@milaGGL milaGGL marked this pull request as ready for review February 19, 2025 15:14
@milaGGL milaGGL requested review from a team as code owners February 19, 2025 15:14
@milaGGL milaGGL requested a review from ehsannas February 19, 2025 15:15
@milaGGL milaGGL added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 19, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 19, 2025
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Feb 20, 2025
@milaGGL milaGGL added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 20, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 20, 2025
@milaGGL milaGGL requested a review from dconeybe February 20, 2025 21:27
@milaGGL milaGGL added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 26, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 26, 2025
}
}

class Random {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use Math.random()?

}
}

describe('CompareUtf8Strings', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You can move the StringPair, StringPairGenerator, StringBuilder, etc classes into this describe scope.

@ehsannas ehsannas assigned milaGGL and unassigned ehsannas Feb 26, 2025
@milaGGL milaGGL merged commit e8777e1 into main Mar 3, 2025
17 checks passed
@milaGGL milaGGL deleted the mila/fix-string-utf8-encoded-comparison branch March 3, 2025 16:16
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/nodejs-firestore API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants