[HOLD bump queue] feat: Split queries with many parameters into multiple queries to avoid too many SQL variables error#804
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6ea41a34f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
too many SQL variables errortoo many SQL variables error
|
@ChavdaSachin can you please review and test the out in the app? |
|
@chrispader Please add manual test steps, test recordings and a linked E/App PR where QA/reviewer will use for testing. |
too many SQL variables errortoo many SQL variables error
|
Please continue testing and reviewing, but I put it on hold as we merged 2 onyx prs that need to be deployed in App first |
|
@mountiny alright, sounds good! Could we trigger an ad-hoc build on the App PR? @fabioh8010 @ChavdaSachin i've added unit tests and updated the SQLite mock to reflect param handling in |
@mountiny
Details
This PR fixes an issue with our SQLite storage provider implementation, where in some cases, very large Onyx DBs with lots of keys would cause queries with
IN (...)to exceed the number of allowed parameters per query (usually more than 32,766, as per https://sqlite.org/limits.html).This PR does the following:
MAX_VARIABLE_NUMBERcompile option once and store it globally.multiGetandremoveItemssplit queries into multiple separate queries, if the number of keys exceeds the max.Related Issues
Expensify/App#94577
Linked E/App PR
Expensify/App#94947
Automated Tests
Manual Tests
To reproduce this error, we must manually add some code to the app locally:
useEffectin https://github.com/Expensify/App/blob/371c63dd33c770f0b2ab6d592d557156c66860ad/src/App.tsx#L73-L144:catchblock to theclearOnyxAndResetAppfunction in https://github.com/Expensify/App/blob/371c63dd33c770f0b2ab6d592d557156c66860ad/src/libs/actions/App.ts#L879-L933[TooManySQLVariablesDemo] Number of keys in Onyx: <number of keys in Onyx>log appears with at least more than 32,766 keys (as this is the configured limit for number of parameters/variables in a query).catch((error) => { ... })block from step 2Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Before fix:
Screen.Recording.2026-06-30.at.17.27.42.mov
After fix:
after.mov