Conversation
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()
There was a problem hiding this comment.
I explored putting _class_for_key and _get_type_order on _BSONType, but found that it coupled the bson.py serialization layer with Firestore query ordering concepts (TypeOrder), and required _BSONType to maintain registry lookups of its own subclasses (which becomes an OOP anti-pattern).
Instead, we mapped the wire keys directly to TypeOrder using a dedicated _BSON_KEY_TO_TYPE_ORDER dictionary in order.py:
bson_order = _BSON_KEY_TO_TYPE_ORDER.get(key)
if bson_order is not None:
return bson_orderThis gives us the clean single-key
| 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
There was a problem hiding this comment.
In Firestore backend query ordering specifications, BSONTimestamp shares the exact same type order category as native Firestore timestamps (TypeOrder.TIMESTAMP).
In _BSON_KEY_TO_TYPE_ORDER, "request_timestamp" directly maps to TypeOrder.TIMESTAMP, and Order.compare_timestamps handles cross-type comparisons between native timestamps (timestamp_value) and BSON timestamps (request_timestamp with seconds and increment).
| 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?
There was a problem hiding this comment.
Exactly right. Casting to float caused overflow/precision loss on large values (e.g. 1e1000).
We updated compare_numbers to extract decimal.Decimal via .to_decimal(), unified NaN handling across both float and Decimal, and convert float to Decimal(str(float)) when comparing across types to prevent Python's TypeError and avoid float overflow. We've also added unit tests for 1e1000 and Decimal NaN comparisons.
| 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)
There was a problem hiding this comment.
Implementing int or float on BSONDecimal128 would still lose precision or overflow for values exceeding standard IEEE 754 float limits (1e1000).
By using a small inline extractor in compare_numbers:
def _to_number(val):
num = decode_value(val, None)
to_decimal = getattr(num, "to_decimal", None)
return to_decimal() if callable(to_decimal) else getattr(num, "value", num)we cleanly unbox native ints/floats, BSONInt32 (.value), and BSONDecimal128 (.to_decimal()) without modifying the public interfaces or type contracts of the BSON classes.
2c6fc33 to
25549bf
Compare
25549bf to
dc953a7
Compare
dc953a7 to
3250d05
Compare
…risons Use _BSON_KEY_TO_TYPE_ORDER dictionary lookup in order.py for O(1) wire key resolution, decoupling bson.py from query ordering. Consolidate cross-type numeric comparisons in compare_numbers with safe Decimal handling and restore compare_doubles to standard float comparisons.
3250d05 to
aeb9997
Compare
Rename ambiguous single-letter variable l to left_val to satisfy flake8 E741 and align expression formatting with ruff.
Context & Problem
Firestore queries require client-side and cross-type value ordering support for BSON types, including BSONMinKey, BSONMaxKey, BSONObjectId, BSONInt32, BSONDecimal128, BSONBinary, BSONRegex, and BSONTimestamp, aligning cross-type ordering with backend specifications.
Summary of Changes
_BSON_KEY_TO_TYPE_ORDERmapping dictionary inorder.pyforTypeOrder.Order.compare_numberswith inline unboxing, unified NaN handling, and safe float-to-Decimalconversion to prevent float overflow and precision loss.Order.compare_doublesto standard float comparisons.Order.compare_timestampssupporting native Firestore timestamps andBSONTimestamp.test_order.pyverifying BSON type ordering,1e1000large Decimal comparisons, DecimalNaNhandling, and wire-key mapping lookups.Verification
pytest tests/unit/v1/test_order.py tests/unit/v1/test_bson.py(112 passed in 1.18s).nox -e lint(all checks passed, 276 files formatted).nox -s mypy-3.11(Success: no issues found in 111 source files).