johnsturgeon/tgfp-web

Watch

Fork 0

Build the allpicks grid from one query #450

Merged

johnsturgeon merged 1 commit from n-1-query-allpicks-page-319 into main

2026-09-02 15:34:07 +02:00

johnsturgeon commented

2026-09-02 15:26:27 +02:00

Owner

The cost

/allpicks is a grid: a row per player, a column per game. Everything it
showed per cell was fetched per cell.

Measured with a seeded 34-player, 16-game week:

TOTAL QUERIES: 722
  715  SELECT ... FROM playergamepick
    3  week
    2  player
    1  game
    1  team

~21 per player: one for picks_for_week (row styling), four for
wins_for_week / losses_for_week / bonus_for_week (the last called twice),
and one per game for pick_for_game_id. pick_for_game_id's comment claims to
"search cached picks first" -- that cache went with WeekInfo, so it is a fresh
SELECT every time.

Games and teams were already fine at one query each.

The change

PlayerPicks (app/models/player_picks.py) is what the page reads: a player,
their picks for the week keyed by game, and the wins/losses/bonus those picks
earned. Not a table -- the picks themselves are still rows in playergamepick.

PlayerPicks.for_week assembles one per player from a single grouped query.
The template's structure is unchanged -- outer loop per player, inner loop per
game -- it just asks player_picks.pick_for(game) instead of the database.

Player._record_from_picks becomes public; it was always a pure fold over a
list of picks.

Deliberately untouched: pick_for_game_id and the per-player record methods.
The standings page still uses them, and one player at a time never had this
problem.

Tests

First coverage of the route. Four say what the page shows -- a row per
player, correct records per player, every pick named, --no pick-- for a
player with none. Those are what make the query change safe to trust.

The fifth is the guard, and it asserts a constant rather than a number:
render 2 players x 2 games, then 12 x 8, and assert the pick-query count is
identical. A threshold would pin today's cost and break on an unrelated change.
Reverting the template to pick_for_game_id / wins_for_week fails it.

Two mutations checked: swapping for_week back to a per-player query fails the
cost test, and making pick_for return None fails the rendering test.

Note

The count lands at 2, not 1. The second is allpicksbase.j2:41 asking whether
the viewing player has picks before showing the page at all -- constant, not
per-cell.

standings.j2 still calls wins_for_week / losses_for_week /
bonus_for_week per player, which is 3 queries each. Smaller, different page,
left alone.

resolves #319

🤖 Generated with Claude Code

## The cost `/allpicks` is a grid: a row per player, a column per game. Everything it showed per cell was fetched per cell. Measured with a seeded 34-player, 16-game week: ``` TOTAL QUERIES: 722 715 SELECT ... FROM playergamepick 3 week 2 player 1 game 1 team ``` ~21 per player: one for `picks_for_week` (row styling), four for `wins_for_week` / `losses_for_week` / `bonus_for_week` (the last called twice), and one per game for `pick_for_game_id`. `pick_for_game_id`'s comment claims to "search cached picks first" -- that cache went with `WeekInfo`, so it is a fresh SELECT every time. Games and teams were already fine at one query each. ## The change `PlayerPicks` (`app/models/player_picks.py`) is what the page reads: a player, their picks for the week keyed by game, and the wins/losses/bonus those picks earned. Not a table -- the picks themselves are still rows in `playergamepick`. `PlayerPicks.for_week` assembles one per player from a single grouped query. The template's structure is unchanged -- outer loop per player, inner loop per game -- it just asks `player_picks.pick_for(game)` instead of the database. `Player._record_from_picks` becomes public; it was always a pure fold over a list of picks. Deliberately untouched: `pick_for_game_id` and the per-player record methods. The standings page still uses them, and one player at a time never had this problem. ## Tests First coverage of the route. Four say what the page *shows* -- a row per player, correct records per player, every pick named, `--no pick--` for a player with none. Those are what make the query change safe to trust. The fifth is the guard, and it asserts a **constant rather than a number**: render 2 players x 2 games, then 12 x 8, and assert the pick-query count is identical. A threshold would pin today's cost and break on an unrelated change. Reverting the template to `pick_for_game_id` / `wins_for_week` fails it. Two mutations checked: swapping `for_week` back to a per-player query fails the cost test, and making `pick_for` return `None` fails the rendering test. ## Note The count lands at 2, not 1. The second is `allpicksbase.j2:41` asking whether the *viewing* player has picks before showing the page at all -- constant, not per-cell. `standings.j2` still calls `wins_for_week` / `losses_for_week` / `bonus_for_week` per player, which is 3 queries each. Smaller, different page, left alone. resolves #319 🤖 Generated with [Claude Code](https://claude.com/claude-code)

johnsturgeon added 1 commit

2026-09-02 15:26:27 +02:00

Build the allpicks grid from one query

Claude Code Review / claude-review (pull_request) Successful in 1m52s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 26s

Details

Tests / pytest (pull_request) Successful in 13s

Details

3f1fdfe0bb

The page reads every player against every game, so anything resolved per cell
was multiplied by both: a query for each player's picks, three more for their
record, and one per cell on top. 34 players against 16 games came to 715
queries against `playergamepick` for a single render -- on the page the whole
pool refreshes at once on a Sunday afternoon.

`PlayerPicks` is what the page actually reads: a player, the picks they made
that week, and the record those picks earned. `for_week` assembles one per
player from a single query, and the template asks it for a pick instead of
asking the database.

`pick_for_game_id` and the per-player record methods are untouched -- the
standings page still uses them, and one player at a time never needed this.

Code by John. Tests by Claude Opus 5, directed and reviewed by John.

resolves #319

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

forgejo-actions commented

2026-09-02 15:28:20 +02:00

This is a clean, well-scoped PR. I've verified the rename is consistent, all callers updated, the new query path, and the template rewrite. One genuine (small) performance/docstring accuracy point stood out.

The change collapses the /allpicks grid from ~700 per-cell queries to a single grouped fetch, and it's done cleanly: PlayerPicks.for_week assembles one row per player from PlayerGamePick.picks_by_player_for_week, the template loops over the pre-built objects, and the _record_from_picks → record_from_picks rename is applied everywhere it's called. The new rendering tests pin what the page shows and the cost test guards the query count as an invariant, which is the right shape for making this refactor trustworthy.

Worth fixing

app/main.py:485 / app/models/player_picks.py:35 — ordering makes the docstring's central claim false and leaves some queries on the table. PlayerPicks.for_week scores each pick by walking pick.game.winning_team → game.home_team/away_team, and its docstring says "the page has already loaded the week's games and every team, so those resolve from the session's identity map." But Team.all_teams(session=...) is called on the next line (app/main.py:488), so at the moment for_week runs the teams are not in the identity map yet. Those relationship accesses lazy-load, issuing roughly one SELECT ... FROM team per final game. It's bounded by games (the result is cached on the shared Game instance), not an N+1 across the grid, so it won't scale with players — but it's real, and simply moving the Team.all_teams(...) call above the PlayerPicks.for_week(...) call eliminates it and makes the docstring accurate. Note the cost test wouldn't catch this either way, since it filters to FROM playergamepick only.

Nits

tests/test_player_model.py:4 — the module docstring still refers to Player._record_from_picks; the method is now public (record_from_picks). Stale reference only, no functional impact.
app/templates/allpicks.j2:55 — row_with_style now receives player_picks.picks (a dict) instead of the old list, but the macro only uses the arg in an is not defined check that can never be true for a passed parameter. Pre-existing dead logic, not introduced here; worth a cleanup pass someday but not part of this change.

Nothing blocking — the correctness of the output is well covered and the perf win is real.

This is a clean, well-scoped PR. I've verified the rename is consistent, all callers updated, the new query path, and the template rewrite. One genuine (small) performance/docstring accuracy point stood out. --- The change collapses the `/allpicks` grid from ~700 per-cell queries to a single grouped fetch, and it's done cleanly: `PlayerPicks.for_week` assembles one row per player from `PlayerGamePick.picks_by_player_for_week`, the template loops over the pre-built objects, and the `_record_from_picks` → `record_from_picks` rename is applied everywhere it's called. The new rendering tests pin what the page shows and the cost test guards the query count as an invariant, which is the right shape for making this refactor trustworthy. ### Worth fixing - `app/main.py:485` / `app/models/player_picks.py:35` — ordering makes the docstring's central claim false and leaves some queries on the table. `PlayerPicks.for_week` scores each pick by walking `pick.game.winning_team` → `game.home_team`/`away_team`, and its docstring says "the page has already loaded the week's games **and every team**, so those resolve from the session's identity map." But `Team.all_teams(session=...)` is called on the *next* line (`app/main.py:488`), so at the moment `for_week` runs the teams are **not** in the identity map yet. Those relationship accesses lazy-load, issuing roughly one `SELECT ... FROM team` per final game. It's bounded by games (the result is cached on the shared `Game` instance), not an N+1 across the grid, so it won't scale with players — but it's real, and simply moving the `Team.all_teams(...)` call above the `PlayerPicks.for_week(...)` call eliminates it and makes the docstring accurate. Note the cost test wouldn't catch this either way, since it filters to `FROM playergamepick` only. ### Nits - `tests/test_player_model.py:4` — the module docstring still refers to `Player._record_from_picks`; the method is now public (`record_from_picks`). Stale reference only, no functional impact. - `app/templates/allpicks.j2:55` — `row_with_style` now receives `player_picks.picks` (a dict) instead of the old list, but the macro only uses the arg in an `is not defined` check that can never be true for a passed parameter. Pre-existing dead logic, not introduced here; worth a cleanup pass someday but not part of this change. Nothing blocking — the correctness of the output is well covered and the perf win is real.

johnsturgeon merged commit be196ef5a2 into main

2026-09-02 15:34:07 +02:00

johnsturgeon deleted branch n-1-query-allpicks-page-319

2026-09-02 15:34:07 +02:00

johnsturgeon referenced this pull request from a commit

2026-09-02 15:34:08 +02:00

Merge pull request 'Build the allpicks grid from one query' (#450) from n-1-query-allpicks-page-319 into main