Repository navigation
fix(sessions): keep integers outside 64 bits exact in SqliteSessionService state - #7577
Somay-kousis wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where SQLite parses integers outside the signed 64-bit range as REAL, leading to precision loss. It introduces a check using the -> operator (available in SQLite 3.38+) to retrieve the exact JSON text representation of large integers and preserve them as exact integers. It also updates the parameter binding in _update_session_state_in_db to use ?1 for the delta JSON. Additionally, unit tests have been added to verify that large integers are stored exactly and survive unrelated state updates. I have no feedback to provide as there are no review comments.
BichengWang
left a comment
There was a problem hiding this comment.
[P2] Guard raw JSON extraction inside each UNION arm
Reviewed dbc39ca2ae354ca75b9b8ba64c9ddd64b0962e90 against current upstream origin/main at e4067dbcc8192615b69d8ca1fc1de17743a71676.
In src/google/adk/sessions/sqlite_session_service.py:84-87, the new source -> fullkey projections run for every key before the outer integer guard. Each lookup searches the source object, so merging an unrelated delta now takes quadratic time even when every value is an ordinary small integer and no value needs this fix. This affects persisted session, user, and app state on subsequent writes.
I reproduced this offline with the exact _MERGE_STATE_SQL assignments extracted from that upstream baseline and the PR head, using state = {f'k{i}': i for i in range(n)} and the single-key delta {'other': 1}. Median of five executions on Python 3.13.16 / SQLite 3.53.4:
| State keys | JSON bytes | Upstream main | PR head |
|---|---|---|---|
| 1,000 | 12,780 | 0.22 ms | 3.23 ms |
| 5,000 | 72,780 | 1.47 ms | 67.70 ms |
| 10,000 | 147,780 | 2.28 ms | 246.49 ms |
| 20,000 | 317,780 | 4.13 ms | 1023.51 ms |
The public SqliteSessionService.append_event path at this head also reproduces the growth: median of three unrelated single-key appends was 46.60 / 177.13 / 622.04 ms for 5k / 10k / 20k state keys, respectively (Python 3.12.15 / SQLite 3.53.1, temporary local database).
Could the extraction be guarded in each inner SELECT, for example CASE WHEN type = 'integer' AND typeof(value) = 'real' THEN source -> fullkey END, so normal values never perform the lookup? Applying only that guard to the SQL in memory reduced the 20k-key case to 7.07 ms, with identical resulting state. All 24 additional SQL round trips for positive/negative out-of-range integers, delta/retained-state paths, and ordinary/dotted/quoted/backslash/Unicode/empty keys passed. A scaling regression check for ordinary state would cover the missing case.
Validation: the PR's new tests passed unchanged (pytest tests/unittests/sessions/test_session_service.py -k large_integer: 36 passed). A broader offline sessions run excluding test_vertex_ai_session_service.py had 578 passed and two failures caused by missing optional Spanner/Agent Platform dependencies; the excluded Vertex test module also could not collect without Google API Core. Full tox was not run. No source or test files were modified.
|
@BichengWang you're right, thanks for measuring it. Every key paid for the Same setup as yours (one-key delta into n small-int keys, median of 5, Python 3.12.13 / SQLite 3.53.4):
Added |
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
_MERGE_STATE_SQLrebuilds the whole state row in SQLite on every state change. SQLite parses an integer outside the signed 64-bit range as REAL, sojson_eachreports it withtype = 'integer'but a REAL value, andjson_group_objectwrites that REAL back. A 64-bit hash stored in state becomes a float, both when it's written and on any later write to another key. #7426 fixed floats in the same SQL and noted this case as not covered.Solution:
For a value whose type is
integerbut whose SQL value is REAL, copy the element's original JSON text with SQLite's->operator instead of the parsed value. The lookup runs only for those values, so merging ordinary state stays linear in the key count. Nothing else in the merge changes.->needs SQLite 3.38+. On older SQLite the expression isNULL, so the merge falls back to today's behavior: no error, and such an integer still reads back rounded there.The delta now binds once as
?1, since the merge reads it in more places.Testing Plan
Unit Tests:
Two new tests in
tests/unittests/sessions/test_session_service.py, run on every session backend and for plain,user:andapp:keys:test_large_integers_survive_unrelated_state_delta(big ints in the initial state, then an unrelated delta) andtest_large_integer_state_delta_is_stored_exactly(big ints written by a delta). They use2**64 - 1and-(2**70) - 1.test_sqlite_state_merge_scales_linearly_with_ordinary_statechecks that a one-key merge into 20k ordinary keys takes under 30x the time of 2k keys (about 10x with the fix).Before the fix, only the SQLite-backed services fail:
After the fix:
With
sqlite3.sqlite_version_infoforced to(3, 37, 2), the merge takes theNULLpath andtest_session_service.pygives12 failed, 374 passed: the same 12 as main, no SQL errors.pyink,isortandmypypass on the changed files.Manual End-to-End (E2E) Tests:
The script from the issue runs an
LlmAgentwith a fake model and three tools on aRunnerwithSqliteSessionService, reloading the session after each turn.Before:
After:
Checklist
Additional context
N/A