Uh oh!
There was an error while loading. Please reload this page.
Chunk queries to avoid going over the sql parameter limit - #72
Conversation
📝 WalkthroughWalkthroughQueries that used unbounded Contains/IN were changed to use EF.Parameter(...) for both snapshot and commit filtering; tests were added that lower SQLite’s variable limit to verify behavior when many IDs/entities are involved. ChangesSQLite parameterization fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Appears to be significantly faster as the snapshot table grows. Only marginally slower on an empty DB.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/SIL.Harmony/Db/CrdtRepository.cs (1)
116-116: 💤 Low valueConsider materializing
commitIdsfor clarity.The deferred execution of
commitIdsis safe here becausecommitsis anICollection<Commit>(already materialized) and isn't modified before the query executes. However, materializing the projection explicitly would make the intent clearer and guard against subtle bugs if the code is refactored later.♻️ Optional materialization for clarity
- var commitIds = commits.Select(c => c.Id);+ var commitIds = commits.Select(c => c.Id).ToArray();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SIL.Harmony/Db/CrdtRepository.cs` at line 116, The projection assigned to commitIds uses deferred LINQ (var commitIds = commits.Select(c => c.Id)); materialize it immediately to make intent explicit and guard against future refactoring by replacing the assignment with an eagerly-evaluated collection (e.g., commitIds = commits.Select(c => c.Id).ToList() or ToArray()); update the code in CrdtRepository.cs where commitIds is defined so downstream usage consumes the materialized list/array instead of an IEnumerable deferred query.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/SIL.Harmony/Db/CrdtRepository.cs`:
- Line 116: The projection assigned to commitIds uses deferred LINQ (var
commitIds = commits.Select(c => c.Id)); materialize it immediately to make
intent explicit and guard against future refactoring by replacing the assignment
with an eagerly-evaluated collection (e.g., commitIds = commits.Select(c =>
c.Id).ToList() or ToArray()); update the code in CrdtRepository.cs where
commitIds is defined so downstream usage consumes the materialized list/array
instead of an IEnumerable deferred query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9f376aa-aed7-446d-b899-404023c37a7d
📒 Files selected for processing (4)
src/SIL.Harmony.Tests/RepositoryTests.cssrc/SIL.Harmony.Tests/SyncTests.cssrc/SIL.Harmony/DataModel.cssrc/SIL.Harmony/Db/CrdtRepository.cs
myieye
left a comment
There was a problem hiding this comment.
Claude benchmarked your chunking fix vs just telling EF to emit a single parameter.
- On an empty DB they're more or less equal (which is arguably the most common scenario that will run into this)
- A single parameter (i.e.
EF.Parameter) quickly becomes faster as the project gains content. E.g. with only 1,000 snapshots/commits it's already twice as fast and its advantage grows linearly with the size of the tables.
So, I refactored to EF.Parameter, which seemed to be the overall winner and is also a much smaller change.
Claude also noticed another spot that would have the same problem on big projects, so I fixed that too.
hahn-kev
commented
Jun 12, 2026
Awesome, I was wondering why it wasn't using json already, I didn't realize you could force it to do that using Parameter. |
Uh oh!
There was an error while loading. Please reload this page.
fixessillsdev/languageforge-lexbox#2339
Summary by CodeRabbit