Repository navigation
fix: list each app once in SQL persister list_app_ids, newest save first - #942
Open
sangkyoonnam wants to merge 2 commits into
Open
sangkyoonnam wants to merge 2 commits into
sangkyoonnam wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CanReader
approved these changes
Oct 4, 2026
CanReader
left a comment
There was a problem hiding this comment.
Looks good to me. I pulled the branch and ran the sqlite and aiosqlite tests, the new ones fail on main and pass here. GROUP BY with MAX(created_at) is the right fix, the old DISTINCT app_id, created_at in the postgres ones was clearly returning the same app more than once.
Small things, none of them blocking:
- Since "newest first" is now kind of a promise for the SQL persisters, maybe worth adding it to the
list_app_idsdocstring so people know they can rely onlist_app_ids(...)[0]like the notebook does. Redis still orders differently, so it's good to be clear about which ones do this. - Agree with leaving the index out of this PR. If someone hits slow queries on a big table, a
(partition_key, app_id, created_at)index can be its own PR.
I didn't run the postgres tests locally, only the sqlite ones.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
|
Thanks for checking it. I added the ordering note to the |
This was referenced Oct 5, 2026
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.
list_app_idson the SQL persisters now returns each app once, ordered by that app's most recent save. Today the Postgres persisters can return the same app once per save, and the SQLite persisters can put an older app first.The Postgres query is
SELECT DISTINCT app_id, created_at ... ORDER BY created_at DESC, so it keeps one row per distinct(app_id, created_at)pair. Savinga-appseq 0,z-appseq 0,a-appseq 1,a-appseq 2 in separate transactions returns['a-app', 'a-app', 'z-app', 'a-app']on main, for both psycopg2 and asyncpg. The SQLite query isSELECT DISTINCT app_id ... ORDER BY created_at DESC, which sorts by whichever row SQLite keeps for each app. Saving a, z, a at distinct timestamps returns['z-app', 'a-app']. The youtube-to-social-media-post notebook picks the app to resume withpersister.list_app_ids("")[0], so on SQLite that can resume the wrong app. This came up while reviewing #920.Changes
GROUP BY app_id ORDER BY MAX(created_at) DESC.list_app_idsdocstrings now say each app is listed once, most recently saved first.How I tested this
created_atvalues and asserts['a-app', 'z-app']. All four fail on main and pass here. The Postgres tests use their own partition key, so rows from the existing tests don't leak in.postgres:15container withBURR_CI_INTEGRATION_TESTS=true: 13 passed.pytest tests --ignore=tests/integrations/persisters/test_postgresql.py --ignore=tests/integrations/test_bip0042_bedrock.py: 682 passed, 4 skipped.pre-commit runon the changed files passes.Notes
sequence_idrather than save time, so ordering still differs across persisters.created_athas one-second resolution, so apps saved within the same second can still come back in either order.MAX(created_at)per app isn't served by the existingcreated_atindex. On an in-memory SQLite table with 100,000 saves across 100 apps, the new query takes about 21 ms against 3 ms for the old one. AfterANALYZEthe planner moves the old query onto thecreated_atindex and both land at roughly 20 ms. A(partition_key, app_id, created_at)index could help large tables, but I haven't measured it and left the schema alone.Checklist