Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the BSONDecimal128 class to represent 128-bit BSON decimals in Firestore, along with corresponding integration and unit tests. The review feedback highlights critical issues in the eq and hash implementations of BSONDecimal128, specifically pointing out a transitivity of equality violation and a hash invariant violation when comparing with decimal.Decimal. The reviewer provides suggestions to resolve these issues by comparing the underlying decimal.Decimal values and recommends adding tests to verify equality transitivity and hash consistency.
1ef7892 to
9b544a8
Compare
9b544a8 to
136d4cc
Compare
d2e6a9b to
1ee01b3
Compare
1ee01b3 to
3b1421c
Compare
3b1421c to
5103b1a
Compare
5103b1a to
fe17e56
Compare
| return self._value | ||
|
|
||
| @property | ||
| def to_decimal(self) -> decimal.Decimal: |
There was a problem hiding this comment.
Is this intended to be a property? The name looks like it should be a method
|
|
||
| def __eq__(self, other: Any) -> bool: | ||
| if isinstance(other, BSONDecimal128): | ||
| if self._value.upper() == "NAN" and other._value.upper() == "NAN": |
There was a problem hiding this comment.
what about float("inf")?
It looks like there are some other special case strings too (e,g, "-NaN"). Maybe we should have some tests around these
| except decimal.InvalidOperation: | ||
| return False | ||
| return NotImplemented | ||
|
|
There was a problem hiding this comment.
We should probably implement __float__, so this type can be treated as a number
fe17e56 to
4799274
Compare
4799274 to
8baccaa
Compare
2 participants
Adds the
BSONDecimal128data type to support 128-bit high-precision decimal field serialization in Google Cloud Firestore according to the official Firestore BSON specification.BSONDecimal128Container Class: Introducesgoogle.cloud.firestore_v1.bson.BSONDecimal128for lossless 128-bit decimal storage (e.g."123.45").str,int,float,decimal.Decimal, or existingBSONDecimal128instances..to_decimalproperty for converting losslessly to Python standard librarydecimal.Decimal._to_map_value()convertingBSONDecimal128into the reserved{"__decimal128__": "<string>"}wire map representation.google.cloud.firestore_v1andgoogle.cloud.firestore).Fixes b/562163604 🦕