Fix: SqlBaseKvStore.__setitem__ duplicated rows instead of updating (rowcount trap left by #4) - #5
Merged
Conversation
…uplicated Update-vs-insert was decided from a SELECT's rowcount, which is -1 on SQLite, so writing an existing key always inserted a duplicate row and reads kept returning the old value. Same rowcount trap #4 fixed in __len__. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thorwhalen
added a commit
that referenced
this pull request
Sep 22, 2026
) The same rowcount trap #4 fixed in SqlBaseKvReader.__len__ and #5 fixed in SqlBaseKvStore.__setitem__ was still in TableRows.__len__, so len() of a TableRows over SQLite raised "ValueError: __len__() should return >= 0". Ask the database for COUNT(*), honouring the row filter. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Post-merge refute review of #4.
Defect: #4 fixed
__len__because a SELECT'srowcountis -1 on SQLite, butSqlBaseKvStore.__setitem__still decides update-vs-insert withresult.rowcount == 1on a SELECT. On SQLite every write to an existing key is an INSERT: the table grows a duplicate row,lengoes up, andstore[k](first matching row) keeps returning the old value. Now that #4 madelenreal, the mapping visibly breaks:store['a'] = new; store['a'] != new.Fix: decide by whether the SELECT returns a row (
.first() is not None). A key that matches rows is updated; otherwise inserted.Tests: new
sqldol/tests/test_kv_store_setitem.py(update-existing, insert-then-update-new). Both fail on master, pass here. Full suite + doctests pass locally (Python 3.10).Not changed here (posted as design notes on #4): the default
missing_key_policy='empty'still makesinanswer True for every key;_mk_column_filterbuilds SQL by string interpolation.Self-reviewed only (the dispatching run disallowed sub-agents).
🤖 Generated with Claude Code