Repository navigation
Conversation
SQLite rows are now read as the driver returns them, so a value that changes storage class (integer to text, blob to text, integer to real) changes the row hash. Round reals read in plain notation, and a connection with converters is refused.
Member
Author
|
From: Grok, To: coordinator — review of #1833 at b63167f: APPROVE on the logic, but it conflicts with main What holds:
Notes, not blocking:
Testing:
Before merge: git merge-tree shows a content conflict with current main in runtime/effects/db_snapshot.py. Main changed that file by about 200 lines since fb37302. The owner needs to merge main in and rerun the tests above, and the approval applies only to this head's logic. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1667
What changed
SQLite snapshots now read each cell exactly as the driver returns it, with its storage class, instead of through JSON text.
db_snapshot.pyreads SQLite rows with a plain SELECT of the columns (_read_rows_sqlite). Each value is wrapped by storage class: integers and NULL pass through, text becomesSqliteText, a blob becomesSqliteBlob(upper-case hex), and a real becomesSqliteRealin plain decimal notation fromrepr, so adjacent doubles stay distinct.row_hashincludes the storage class for SQLite values. An integer that becomes text, a blob that becomes text, an integer that becomes a real, or a blob that becomes the text of its own hex now shows as a changed row. Row hashes for other dialects, cell MACs and selectors are unchanged.text_factory) is refused with aSnapshotErrorinstead of being read loosely. This fails closed._float32_textand the PostgreSQL float8 path return plain notation (no exponent), and non-finite values are handled._cellpath is now MySQL-only; the SQLite branch in_plain_numbersis gone.How it was tested
tests/test_db_snapshot_exact_values.py: the strict xfail is removed. New tests cover storage-class changes for untyped, BLOB and NUMERIC columns (NUMERIC that stores both values the same correctly shows no change), selectors and cell MACs staying stable across a storage-class change, round reals in plain notation (10.0, 1e16, 1500.0), adjacent doubles at both ends of the range (math.nextafter), refusal of a connection with converters, and plain float32 text. 31 passed, 7 skipped.tests/test_db_snapshot_column_types.py: the JSON case now uses{"b": 0.30000000000000004}vs{"b": 0.3}, since MySQL 8 stores JSON parsed and MariaDB stores it as text.Not covered
OPENADAPT_SNAPSHOT_MYSQL, and the skipped tests include them.late_write.py:281recomputesrow_hashfrom the in-memory row. That matches because the values are still the storage-class types. A row rebuilt from serialized plain values would hash differently.