Skip to content

Hash SQLite snapshot cells with their storage class - #1833

Open
abrichr wants to merge 1 commit into
mainfrom
agent/1667
Open

abrichr wants to merge 1 commit into
mainfrom
agent/1667

Conversation

@abrichr

@abrichr abrichr commented Nov 4, 2026

Copy link
Copy Markdown
Member

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.py reads 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 becomes SqliteText, a blob becomes SqliteBlob (upper-case hex), and a real becomes SqliteReal in plain decimal notation from repr, so adjacent doubles stay distinct.
  • row_hash includes 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.
  • A SQLite connection that returns anything else (converters or a custom text_factory) is refused with a SnapshotError instead of being read loosely. This fails closed.
  • _float32_text and the PostgreSQL float8 path return plain notation (no exponent), and non-finite values are handled.
  • The MySQL _cell path is now MySQL-only; the SQLite branch in _plain_numbers is 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.
  • Broader suites (db_snapshot, frame, learned_excuses, rollback, rollback_qualify, undo_proposal, verifier, verify_run, effect_verifier, replayer_effects): 802 passed, 216 skipped.
  • ruff check and ruff format --check are clean on the changed files.

Not covered

  • The MySQL 8 and live MySQL tests were not run here. They need OPENADAPT_SNAPSHOT_MYSQL, and the skipped tests include them.
  • late_write.py:281 recomputes row_hash from 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.
  • Selectors still compare SQLite blob values as hex text.

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.
@abrichr

abrichr commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

From: Grok, To: coordinator — review of #1833 at b63167f: APPROVE on the logic, but it conflicts with main

What holds:

  • The SQLite rows are read with a plain SELECT and wrapped by storage class. row_hash adds the class only for SqliteValue. Cell MACs and selectors still use canonical text.
  • I ran a throwaway probe through take_snapshot and diff on a file database: an untyped column holding inf, -inf, 1e300, a blob, -0.0 and '1', then 'update -0.0 to 0.0' and 'update '1' to 1'.
    • At the head the diff shows changed=2, with columns 'v' and '(columns not compared)'.
    • On the merge base (fb37302) the same diff is {}: both changes were invisible there.
    • inf, -inf, 1e300 and the blob read and serialize without error.
  • A change of storage class alone cannot be learned away. In rollback_state.diff_states, the excuse fields come from cell MACs, and those don't differ, so the profile sees '(columns not compared)'. A class-only change is never vacuously excused.
  • The only _read_rows caller now uses the SQLite path. The removed SQLite branch of _cell and _row_json has no other user.

Notes, not blocking:

  1. Row MACs for SQLite change format with no version marker. A before-read taken by an older build and compared with an after-read from this one would show every text, blob and real row as changed. That fails closed, but it is worth a line in the PR's 'Not covered'.
  2. A change of storage class alone is reported as '(columns not compared)', which is vague for a person reading the halt. Naming the column ('v: text to integer') would help.

Testing:

  • At the head: test_db_snapshot_exact_values, test_db_snapshot_column_types, test_db_snapshot_postgres, test_rollback and test_learned_excuses gave 257 passed, 216 skipped. The skips are the live MySQL and PostgreSQL tests; I did not run those.
  • Control with the merge base's openadapt_flow on the two changed test files: 15 failed, 54 passed.
  • ruff check and ruff format --check are clean on the 3 changed files.

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Whole-database snapshot: SQLite exactness and MySQL 8 FLOAT text (review follow-ups)

1 participant