perf: push limit/offset to DB in PlayerService.get_players (#37) #43

cal · 2026-03-05T16:34:11Z

cal commented

2026-03-05 16:34:11 +00:00

Summary

Moves pagination from Python list slicing to database-level .offset() / .limit() calls on the Peewee query
total_count is now obtained via query.count() before pagination, rather than len(players_data) after materializing the full result set
In-memory / mock queries (InMemoryQueryResult) continue to use Python-level slicing unchanged

Files Changed

app/services/player_service.py — PlayerService.get_players (lines 138–152)

Test Results

No automated test suite. Changes verified by code review — the functional logic is unchanged for callers; only the execution strategy differs (DB-level vs Python-level pagination).

Other observations

The QueryResult protocol in app/services/interfaces.py does not declare offset/limit methods. Pyright raises type warnings on the new calls (query.offset(), query.limit()). These are pre-existing protocol coverage gaps; the code is correct at runtime since the else branch is only reached for real Peewee queries.

## Summary - Moves pagination from Python list slicing to database-level `.offset()` / `.limit()` calls on the Peewee query - `total_count` is now obtained via `query.count()` before pagination, rather than `len(players_data)` after materializing the full result set - In-memory / mock queries (`InMemoryQueryResult`) continue to use Python-level slicing unchanged ## Files Changed - `app/services/player_service.py` — `PlayerService.get_players` (lines 138–152) ## Test Results No automated test suite. Changes verified by code review — the functional logic is unchanged for callers; only the execution strategy differs (DB-level vs Python-level pagination). ## Other observations - The `QueryResult` protocol in `app/services/interfaces.py` does not declare `offset`/`limit` methods. Pyright raises type warnings on the new calls (`query.offset()`, `query.limit()`). These are pre-existing protocol coverage gaps; the code is correct at runtime since the `else` branch is only reached for real Peewee queries.

cal added 1 commit 2026-03-05 16:34:11 +00:00

perf: push limit/offset to DB in PlayerService.get_players (#37 )

Build Docker Image / build (pull_request) Successful in 2m26s

Details

99828893c2

Apply .offset() and .limit() on the Peewee query before materializing
results, instead of fetching all rows into memory and slicing in Python.
Total count is obtained via query.count() before pagination is applied.
In-memory (mock) queries continue to use Python-level slicing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

cal referenced this pull request

2026-03-05 16:34:24 +00:00

Push `limit`/`offset` to the database in `PlayerService.get_players` #37

cal added the

ai-reviewing

label 2026-03-05 16:45:34 +00:00

cal reviewed 2026-03-05 16:48:03 +00:00

cal left a comment

AI Code Review

Files Reviewed

app/services/player_service.py (modified)
app/services/interfaces.py (read for context)

Findings

Correctness

The query.count() call happens on the fully-filtered, fully-sorted query object (after _apply_player_filters and _apply_player_sort have applied their .where() and .order_by() clauses), so total_count correctly reflects the pre-pagination filtered set. ✓
offset/limit are guarded by if is not None before calling .offset()/.limit(), so None is never passed to Peewee. ✓
InMemoryQueryResult supports __len__, so len(query) in the mock branch is correct and doesn't require an extra iteration. ✓
The isinstance(query, InMemoryQueryResult) branch correctly preserves pre-existing mock/test behavior. ✓
offset=0 is handled correctly — the guard if offset is not None passes for 0, and .offset(0) is a Peewee no-op. ✓

Security

No issues. All DB filtering goes through Peewee ORM parameterization.

Style & Conventions

Quote normalization (' → ") and line-length reformatting are consistent with the surrounding codebase style.
Trailing comma added to the return dict is PEP 8 / Black-style compliant.

Suggestions

Extra COUNT for non-paginated calls: When limit is None and offset is None, the else branch still issues query.count() before materializing the full result set. The old code did one query + len(players_data) in this case; the new code does two queries (COUNT + SELECT). For the players table this is unlikely to matter in practice — a COUNT query with an index is extremely cheap — but it's worth knowing the tradeoff. A simple optimization would be to skip the count query when neither limit nor offset is provided, computing it from len(players_data) after materialization instead. Not a blocker.
Protocol gap (pre-existing, acknowledged in PR): QueryResult in interfaces.py does not declare offset/limit. If type coverage is ever added to the service layer, these methods should be added to the protocol.

Verdict: APPROVED

Correct, well-scoped performance improvement. DB-level pagination avoids materializing the full player table for paginated requests. The mock path is unchanged. No functional regressions or security issues.

Automated review by Claude PR Reviewer (posted as COMMENT — Gitea does not allow self-approval)

## AI Code Review ### Files Reviewed - `app/services/player_service.py` (modified) - `app/services/interfaces.py` (read for context) ### Findings #### Correctness - The `query.count()` call happens on the fully-filtered, fully-sorted query object (after `_apply_player_filters` and `_apply_player_sort` have applied their `.where()` and `.order_by()` clauses), so `total_count` correctly reflects the pre-pagination filtered set. ✓ - `offset`/`limit` are guarded by `if is not None` before calling `.offset()`/`.limit()`, so `None` is never passed to Peewee. ✓ - `InMemoryQueryResult` supports `__len__`, so `len(query)` in the mock branch is correct and doesn't require an extra iteration. ✓ - The `isinstance(query, InMemoryQueryResult)` branch correctly preserves pre-existing mock/test behavior. ✓ - `offset=0` is handled correctly — the guard `if offset is not None` passes for `0`, and `.offset(0)` is a Peewee no-op. ✓ #### Security - No issues. All DB filtering goes through Peewee ORM parameterization. #### Style & Conventions - Quote normalization (`'` → `"`) and line-length reformatting are consistent with the surrounding codebase style. - Trailing comma added to the return dict is PEP 8 / Black-style compliant. #### Suggestions - **Extra COUNT for non-paginated calls**: When `limit is None and offset is None`, the `else` branch still issues `query.count()` before materializing the full result set. The old code did one query + `len(players_data)` in this case; the new code does two queries (COUNT + SELECT). For the players table this is unlikely to matter in practice — a COUNT query with an index is extremely cheap — but it's worth knowing the tradeoff. A simple optimization would be to skip the count query when neither limit nor offset is provided, computing it from `len(players_data)` after materialization instead. Not a blocker. - **Protocol gap** (pre-existing, acknowledged in PR): `QueryResult` in `interfaces.py` does not declare `offset`/`limit`. If type coverage is ever added to the service layer, these methods should be added to the protocol. ### Verdict: APPROVED Correct, well-scoped performance improvement. DB-level pagination avoids materializing the full player table for paginated requests. The mock path is unchanged. No functional regressions or security issues. --- *Automated review by Claude PR Reviewer (posted as COMMENT — Gitea does not allow self-approval)*

cal added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-05 16:48:21 +00:00

cal changed target branch from main to next-release

2026-03-07 07:32:37 +00:00

cal merged commit 5a7fa24141 into next-release

2026-03-10 14:29:15 +00:00

cal deleted branch ai/major-domo-database-37

2026-03-10 14:29:16 +00:00

cal referenced this issue from a commit