support max_total_entries option
#116
Conversation
WalkthroughThe changes introduce a private helper function to conditionally apply a limit to Ecto queries based on a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ScrivenerPaginator
participant EctoQuery
Caller->>ScrivenerPaginator: paginate(query, opts)
ScrivenerPaginator->>EctoQuery: maybe_limit_by_max_total_entries(query, opts)
EctoQuery-->>ScrivenerPaginator: query (possibly limited)
ScrivenerPaginator->>EctoQuery: aggregate(query, :count, field)
EctoQuery-->>ScrivenerPaginator: total_entries (capped if max_total_entries set)
ScrivenerPaginator-->>Caller: paginated result (with capped total_entries)
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/scrivener/paginater/ecto/query.ex (1)
82-94: Solid implementation with proper validation.The helper function correctly validates that max_total_entries is a positive integer before applying the limit. The error message is clear and descriptive, which will help developers understand how to correctly use this option.
One consideration: While raising an ArgumentError is appropriate for obvious programming errors, you might consider adding a note to the documentation about this validation to help users avoid runtime errors.
Consider adding documentation (either in a module doc or function doc) to clearly specify the expected type and constraints for the max_total_entries option.
test/scrivener/paginator/ecto/query_test.exs (1)
236-260: Consider adding a test for invalid max_total_entries.While your current tests cover the happy path well, I recommend adding a test that verifies the ArgumentError is raised when an invalid max_total_entries value is provided (such as a negative number or non-integer).
+ test "raises ArgumentError when max_total_entries is invalid" do + assert_raise ArgumentError, fn -> + Post + |> Post.published() + |> Scrivener.Ecto.Repo.paginate(options: [max_total_entries: -1]) + end + + assert_raise ArgumentError, fn -> + Post + |> Post.published() + |> Scrivener.Ecto.Repo.paginate(options: [max_total_entries: "not_an_integer"]) + end + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/scrivener/paginater/ecto/query.ex(1 hunks)test/scrivener/paginator/ecto/query_test.exs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/scrivener/paginator/ecto/query_test.exs (2)
test/support/post.ex (1)
published(16-18)lib/scrivener/paginater/ecto/query.ex (1)
paginate(9-36)
🔇 Additional comments (4)
lib/scrivener/paginater/ecto/query.ex (2)
70-70: Efficient improvement to grouped query aggregation.The addition of
maybe_limit_by_max_total_entriesin the pipeline ensures that grouped queries also benefit from the max_total_entries optimization, which is important for performance when dealing with large datasets.
77-80: Well-structured refactoring of the aggregate function.Good refactoring of the simple aggregate function to include the max_total_entries limit option. The implementation maintains a clean pipeline structure and effectively implements the core performance optimization described in the PR objectives.
test/scrivener/paginator/ecto/query_test.exs (2)
236-246: Good basic test coverage for the new feature.This test effectively verifies the core functionality - that max_total_entries properly limits the total_entries count while not affecting the number of entries returned on the current page. The assertion on line 245 correctly confirms that the total_entries is capped at 4 as specified.
248-260: Excellent test for complex query scenarios.This test provides valuable coverage for the more complex use case involving inner joins, group by clauses, and tuple selects. It verifies that the pagination mechanism properly respects the max_total_entries option even in these advanced scenarios.
The assertions on lines 258-259 correctly verify both that the entries returned are unaffected (2 entries) while the total_entries count is capped at 1 as specified by max_total_entries.
I'm finding that for paginating tables with hundreds of thousands of rows, it can take a long time to simply count the records in the table - even if all filters are indexed properly.
I had the thought that we could optionally apply a limit when counting the total rows. Of course this could result in incorrect
total_entriesandtotal_pages, but my thought is that this can be handled in a UI with something like "viewing page 1 of 100+ pages" and "viewing 25 of 1000+ entries".Summary by CodeRabbit
New Features
Tests