fix(sessions): handle SQLite float timestamps in PreciseTimestamp.result_processor#6355
Open
abhiramArise wants to merge 6 commits into
Open
fix(sessions): handle SQLite float timestamps in PreciseTimestamp.result_processor#6355abhiramArise wants to merge 6 commits into
abhiramArise wants to merge 6 commits into
Conversation
…e float timestamps Fixes google#6352. SQLite stores DateTime columns as REAL (Unix epoch float). Without a result-value converter, reading them back raised TypeError: fromisoformat: argument must be str. This adds the missing converter, matching the naive local-time convention already used on the write side in StorageEvent.from_event.
…lue on PreciseTimestamp TypeDecorator.process_result_value runs after the underlying impl's own result processor, which already fails when it receives a raw float before process_result_value can intercept it. Overriding result_processor directly runs before the impl's processor, allowing floats to be handled explicitly while delegating to the impl's own processor otherwise. Also adds a regression test that forces a raw REAL-affinity float into the events table via raw SQL to reproduce the original failure, since values written through the normal ORM path are already serialized as text and do not trigger the bug. Thanks to @surajksharma07 for catching the process_result_value issue in review on google#6352.
…n-timestamp-float
Collaborator
|
Hi @abhiramArise , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please fix the failing CI mypy-check tests before we can proceed with the review. |
Adds type hints to PreciseTimestamp.result_processor and its nested process() function, and uses impl_instance instead of impl to satisfy mypy's expectation of an instance rather than a class reference when calling result_processor on the underlying type. Verified this introduces zero new mypy errors: the 11 remaining errors on this file are identical (same messages) on unmodified main, all in unrelated pre-existing methods this change doesn't touch.
Author
|
@rohityan Fixed — added type annotations to result_processor (and its nested Looks like the new CI run needs maintainer approval to start (Action required)
|
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.
Fixes #6352
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:
DatabaseSessionService.get_session()raisedTypeError: fromisoformat: argument must be strwhen reading rows where thetimestampcolumn held a raw SQLite REAL-affinity float instead of the text format SQLAlchemy'sDateTimetype normally writes.Solution:
PreciseTimestamp(aTypeDecoratorwrapping SQLAlchemyDateTime) had no override for handling this case. My first attempt usedprocess_result_value, but that hook runs after the underlying impl's own result processor — which already fails trying to parse a float as ISO text beforeprocess_result_valuecan intercept it (thanks to @surajksharma07 for catching this in review). Fixed by overridingresult_processordirectly instead, which runs before the impl's own processor. It handlesint/floatvalues explicitly viadatetime.fromtimestamp(), matching the naive local-time convention already used on the write side inStorageEvent.from_event, and delegates to the impl's normal processor for everything else.Testing Plan
Unit Tests:
Added
test_database_session_service_sqlite_file_timestamp_read_after_reopenintests/unittests/sessions/test_session_service.py. Verified the test is meaningful: confirmed it FAILS with the originalTypeError: fromisoformat: argument must be strwhen the fix is reverted, and PASSES when the fix is in place.Note: writing through the normal ORM path (
append_event→StorageEvent.from_event) doesn't reproduce this bug, since SQLAlchemy's SQLiteDateTimetype already serializes to text before the value reaches the column. The test writes a row normally, then directly overwrites thetimestampcolumn via raw SQL to force a real float into it (simulating a REAL-affinity value from any write path that bypasses SQLAlchemy's serialization), then confirmsget_session()reads it back correctly.Ran the full session test suite locally:
pytest -q tests/unittests/sessions/test_session_service.py
→ 133 passed, 4 failed (all pre-existing and unrelated to this change, confirmed present on unmodified main as well: a flaky microsecond-timing assertion in
test_create_get_session, a Windows-only near-epochOSErrorin an unrelated parametrized test, an intermittent test-order state-leakage failure intest_append_event_calls_rollback_on_commit_failure, and a missing optionalvertexaidependency in my local environment).Also ran the full multi-version suite via
toxas required by CONTRIBUTING.md:I was unable to run the full tox multi-version suite locally due to environment constraints on Windows. I ran the test suite locally with pytest across two separate environments (a global environment with SQLAlchemy 2.0.30, and this repo's own isolated uv-managed environment resolving to SQLAlchemy 2.0.51) with consistent results in both — the new regression test passes, and the only failures present are the same ones I independently confirmed exist on unmodified main (see above). Relying on CI's tox matrix to confirm full cross-version compatibility.
Manual End-to-End (E2E) Tests:
Not performed beyond the automated regression test above, which reproduces the exact failure mode (a raw float reaching the
timestampcolumn) and confirms the read path now succeeds.Note: the exact real-world trigger for how a raw float ends up in that column (vs. SQLAlchemy's normal text serialization) is still being confirmed with the original reporter on the issue thread. This fix correctly handles the float case whenever it occurs, regardless of how it's produced.
Checklist
Additional context
N/A