perf(persistence): Move thread messages pagination to SQL#2750
perf(persistence): Move thread messages pagination to SQL#2750VelikovPetar wants to merge 4 commits into
Conversation
Back-port of #2688 to v9. Refactors `MessageDao.getThreadMessagesByParentId` to filter and paginate thread replies at the SQL level instead of loading the full thread into memory, and adds support for all four `PaginationParams` cursor variants (`lessThan` / `lessThanOrEqual` / `greaterThan` / `greaterThanOrEqual`) with a `(createdAt, id)` tiebreaker. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v9 #2750 +/- ##
=====================================
Coverage ? 66.33%
=====================================
Files ? 429
Lines ? 27120
Branches ? 0
=====================================
Hits ? 17989
Misses ? 9131
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if (options != null) { | ||
| query.limit(options.limit); | ||
| } |
There was a problem hiding this comment.
Is this intended?
- limit: 0 behavior changed. Old code: if (limit != null && limit > 0) return mutable.take(limit)... ; return mutable; — so limit: 0 was treated as "unlimited" and returned everything. New code: if (options != null) query.limit(options.limit); — LIMIT 0 in SQL returns zero rows. Since PaginationParams.limit defaults to 10 and no call site in this repo passes 0 explicitly, this is unlikely to matter in practice, but it is a silent behavior change from the pre-port implementation.
There was a problem hiding this comment.
Hmm good catch - I can't say that it was intentional.
It is a bit tricky to say what should be correct here:
- Does calling the method with
options: nullmeans return all thread replies? - Does calling the method with
options: PaginationParams(limit: 0)also means return all messages?
While I prefer the first approach a bit more, perhaps we should revert this to the pre-refactor behaviour. While the default SDK behaviour should not be affected by this, it could potentially affect a customer calling the persistenceClient.getReplies method directly.
(this would also require a fix on master as well, because there we have already merged this change - #2688)
# Conflicts: # packages/stream_chat_persistence/CHANGELOG.md
2 participants
Submit a pull request
Linear: FLU-487
Github Issue: #
CLA
Description of the pull request
Back-port of #2688 to v9. Refactors
MessageDao.getThreadMessagesByParentIdto filter and paginate thread replies at the SQL level instead of loading the full thread into memory, and adds support for all fourPaginationParamscursor variants (lessThan/lessThanOrEqual/greaterThan/greaterThanOrEqual) with a(createdAt, id)tiebreaker.