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 support for ordering and comparing BSON types (such as BSON min/max keys, object IDs, binaries, regexes, timestamps, and numbers) in Firestore. Feedback on these changes suggests adding defensive checks for keys in BSON regex and timestamp maps to prevent potential KeyError exceptions, as well as simplifying the number comparison logic by removing redundant float attribute checks.
b92472b to
9e8a85b
Compare
9e8a85b to
2c6fc33
Compare
| return TypeOrder.BSON_REGEX | ||
| if key == "__request_timestamp__": | ||
| return TypeOrder.TIMESTAMP | ||
| if "__type__" in fields and fields["__type__"].string_value == "__vector__": |
There was a problem hiding this comment.
It seems like there should be a cleaner way to do this
You have to do a similar key->BSONType mapping in the last PR. Maybe we can do something similar, and add an extra cls._get_type_order() field to each BSONType?
Then you could just do something like BSONType._class_for_key(key)._get_type_order()
| TypeOrder.BSON_MIN_KEY: 1, | ||
| TypeOrder.BOOLEAN: 2, | ||
| TypeOrder.NUMBER: 3, | ||
| TypeOrder.TIMESTAMP: 4, |
There was a problem hiding this comment.
BSON_TIMESTAMP seems to be missing
| left_val = left_val.value | ||
| if hasattr(right_val, "value"): | ||
| right_val = right_val.value | ||
| return Order.compare_doubles(float(left_val), float(right_val)) |
There was a problem hiding this comment.
It looks like BSONDecimal can hold larger values than float. maybe we should use decimal.Decimal here?
| if hasattr(left_val, "value"): | ||
| left_val = left_val.value | ||
| if hasattr(right_val, "value"): | ||
| right_val = right_val.value |
There was a problem hiding this comment.
This wouldn't be needed if we implement __int__ and __float__ in the BSON types, so they are automatically treated as numbers (We would still need to compare decimals for BSONDecimal though)
2c6fc33 to
25549bf
Compare
25549bf to
dc953a7
Compare
dc953a7 to
3250d05
Compare
2 participants
No description provided.