Add reproduction for Persistence issue - #6869
Conversation
🦋 Changeset detectedLatest commit: 9517272 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — one new reproduction test for the SQL getMany duplicate-key bug.
PersistenceDuplicateKeys.repro.test.ts: sets"key"→{ value: 1 }in an in-memory SQLite backing store, then callsgetMany(["key", "key"])and asserts[{ value: 1 }, { value: 1 }]. The test correctly operates at theBackingPersistenceStorelevel where the contract is one result per input key position.
The assertion is precise (deepStrictEqual) and targets the exact gap in the SQL implementations: both layerBackingSql and layerBackingSqlMultiTable use keys.indexOf(row.id) to map rows back to input positions, and indexOf only finds the first occurrence — leaving duplicate-key slots as undefined.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior review at 721dedc.
Persistence.ts: replacedkeys.indexOf(row.id)with aMap<string, object>lookup in bothlayerBackingSqlandlayerBackingSqlMultiTablegetManyimplementations, then reconstructed the result viakeys.map(key => values.get(key)). This correctly preserves duplicate-key positions instead of leaving trailing slots asundefined.calm-keys-repeat.md: added a patch changeset foreffect.
The Map-based approach handles duplicates naturally and also improves the algorithmic complexity from O(n×m) to O(n+m). Edge cases are unchanged: missing keys still produce undefined slots, orphan rows from the query are silently dropped as before, and the Arr.NonEmptyArray cast remains valid.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
| @@ -0,0 +1,20 @@ | |||
| import { SqliteClient } from "@effect/sql-sqlite-node" | |||
There was a problem hiding this comment.
Remove this test file and merge with the main one.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior review at bd6e449.
PersistenceDuplicateKeys.repro.test.tsdeleted, test moved intoPersistence.test.ts: the standalone reproduction test file has been removed and thegetMany with duplicate keysregression test is now inside the parameterizedsuite()helper. This means it runs against bothlayerBackingSqlandlayerBackingSqlMultiTableinstead of onlylayerBackingSql. The test logic and store name are unchanged.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
Fix SQL-backed Persistence
getManyso it returns one positional result for every input key, including repeated keys, matching the memory, Redis, and KVS implementations.The SQL implementations now decode query rows into a lookup and reconstruct the result in the original input order. The regression case is part of the main SQLite Persistence suite and runs against both SQL backing variants.
Closes EFF-308
Verification