From f5b6980913664f69695e7b0bcb0f0fbffbb4c35d Mon Sep 17 00:00:00 2001 From: Hal Blackburn Date: Thu, 26 Dec 2024 05:37:00 +0000 Subject: [PATCH 1/2] fix: distinguish -0.0 from 0.0 when caching pack_key test_increment_packed_key was failing because hypothesis generated an example with -0.0, 0.0 which failed because pack_key was caching -0.0 and 0.0 as being equivalent, so the (cached) packed representation of both was the same. pack_key() now distinguishes negative/positive zero float values when caching packed keys. --- src/denokv/datapath.py | 20 +++++++++++++++++--- test/test_datapath.py | 21 +++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/denokv/datapath.py b/src/denokv/datapath.py index aa43421..94ba7ab 100644 --- a/src/denokv/datapath.py +++ b/src/denokv/datapath.py @@ -15,7 +15,9 @@ from typing import Container from typing import Final from typing import Protocol +from typing import Type from typing import TypedDict +from typing import Union from typing import overload from typing import runtime_checkable @@ -49,7 +51,7 @@ from typing_extensions import TypeVar from typing_extensions import Unpack - KvKeyPiece: TypeAlias = "str | bytes | int | float | bool" + KvKeyPiece: TypeAlias = Union[str, bytes, int, float, bool] KvKeyPieceT = TypeVar("KvKeyPieceT", bound=KvKeyPiece, default=KvKeyPiece) KvKeyTuple: TypeAlias = tuple[KvKeyPieceT, ...] @@ -459,8 +461,10 @@ def parse_protobuf_kv_entry( return Ok((key, value, raw.versionstamp)) +_PackedKeyCacheKey: TypeAlias = tuple[Type[KvKeyPiece], Union[str, bytes, int, bool]] +_ieee_binary64: Callable[[float], bytes] = struct.Struct("d").pack _PACK_KEY_CACHE_LIMIT = 128 -_PACK_KEY_CACHE: dict[tuple[tuple[type[KvKeyPiece], ...], KvKeyTuple], bytes] = {} +_PACK_KEY_CACHE: dict[tuple[_PackedKeyCacheKey, ...], bytes] = {} def pack_key(key: AnyKvKey) -> bytes: @@ -488,7 +492,17 @@ def pack_key(key: AnyKvKey) -> bytes: return key.kv_key_bytes() cache = _PACK_KEY_CACHE - cache_key = tuple([type(x) for x in key]), tuple(key) + + cache_key: tuple[_PackedKeyCacheKey, ...] = tuple( + [ + # -0.0 and 0.0 needs to be cached separately, but Python treats them + # as equal. So we use the binary float representation as the cache + # key. (This is slightly faster than using copysign, and potentially + # handles other non-canonical float equality differences.) + (float, _ieee_binary64(x)) if isinstance(x, float) else (type(x), x) + for x in key + ] + ) packed_key = cache.get(cache_key) if packed_key: return packed_key diff --git a/test/test_datapath.py b/test/test_datapath.py index 33335dc..f73d53a 100644 --- a/test/test_datapath.py +++ b/test/test_datapath.py @@ -20,6 +20,7 @@ from aiohttp.test_utils import TestClient as _TestClient from fdb.tuple import pack from fdb.tuple import unpack +from hypothesis import example from hypothesis import given from hypothesis import strategies as st from typing_extensions import TypeAlias @@ -789,6 +790,25 @@ def test_pack_key_cache() -> None: assert len(datapath._PACK_KEY_CACHE) == datapath._PACK_KEY_CACHE_LIMIT +all_floats = st.floats(allow_nan=True, allow_infinity=True, allow_subnormal=True) + + +@given(f1=all_floats, f2=all_floats) +@example(f1=-0.0, f2=0.0) +def test_pack_key_cache__distinguishes_float_values(f1: float, f2: float) -> None: + print(f"equal={pack((f1,)) == pack((f2,))}", f1, f2) + for _i in range(2): + fdb_packed1 = pack((f1,)) + cached_packed1 = pack_key((f1,)) + fdb_packed2 = pack((f2,)) + cached_packed2 = pack_key((f2,)) + + if fdb_packed1 == fdb_packed2: + assert cached_packed1 == cached_packed2 + else: + assert cached_packed1 != cached_packed2 + + def test_pack_key__rejects_unsupported_types() -> None: # Only supported types are allowed, not all types allowed by FoundationDB # key tuples. @@ -813,6 +833,7 @@ def test_increment_packed_key__behaviour() -> None: @given(pieces=ordered_kv_key_pieces_of_same_type) +@example((-0.0, 0.0)) def test_increment_packed_key( pieces: tuple[float, float] | tuple[str, str] From ac235e632d8a3093aafb985da7a210cce75909fd Mon Sep 17 00:00:00 2001 From: Hal Blackburn Date: Thu, 26 Dec 2024 03:27:21 +0000 Subject: [PATCH 2/2] feat: support bytes(KvKey(...)) KvKey now implements __bytes__() to return the packed key value. --- src/denokv/kv_keys.py | 3 +++ test/test_kv_keys__kvkey.py | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/src/denokv/kv_keys.py b/src/denokv/kv_keys.py index 456f78f..cba2dad 100644 --- a/src/denokv/kv_keys.py +++ b/src/denokv/kv_keys.py @@ -117,6 +117,9 @@ def kv_key_bytes(self) -> bytes: self._packed = packed = pack(self._unpacked) return packed + def __bytes__(self) -> bytes: + return self.kv_key_bytes() + @classmethod def from_kv_key_bytes(cls, packed_key: bytes) -> DefaultKvKey: """Create a KvKey by unpacking a packed key.""" diff --git a/test/test_kv_keys__kvkey.py b/test/test_kv_keys__kvkey.py index 9a4630b..67bf47e 100644 --- a/test/test_kv_keys__kvkey.py +++ b/test/test_kv_keys__kvkey.py @@ -76,6 +76,10 @@ def test_kv_key_bytes() -> None: assert KvKey("foo", 1).kv_key_bytes() == pack_key(("foo", 1)) +def test_instances_implement_SupportsBytes() -> None: + assert bytes(KvKey("foo", 1)) == pack_key(("foo", 1)) + + @pytest.mark.parametrize("l", [-1, 0, 1, -1.0, 0.0, 1.0, "a", "b"]) @pytest.mark.parametrize("r", [-1, 0, 1, -1.0, 0.0, 1.0, "a", "b"]) def test_order_comparisons(l: KvKeyPiece, r: KvKeyPiece) -> None: # noqa: E741