cal/major-domo-database
1
0
Fork 0
Code Issues 25 Pull Requests 25 Actions Packages Projects Releases Wiki Activity

fix: add missing indexes on FK columns in stratplay and stratgame (#74) #95

Open
Claude wants to merge 1 commits from issue/74-add-missing-indexes-on-foreign-key-columns-in-high into main
pull from: issue/74-add-missing-indexes-on-foreign-key-columns-in-high
merge into: cal:main
Conversation 0 Commits 1 Files Changed 2 +820 -492
Claude commented 2026-03-27 10:06:58 +00:00
Collaborator
Copy Link

Closes #74

Changes

app/db_engine.py — Added Meta classes with indexes declarations to StratGame and StratPlay:

  • StratGame.Meta.indexes: single-column index on season; composite index on (season, week, game_num)
  • StratPlay.Meta.indexes: single-column indexes on game (FK→stratgame), batter (FK→player), pitcher (FK→player), runner (FK→player)

migrations/2026-03-27_add_stratplay_stratgame_indexes.sql — New migration using CREATE INDEX IF NOT EXISTS for all 6 indexes. Safe to run on existing databases.

Why

PostgreSQL does not auto-index foreign key columns. stratplay and stratgame are the highest-volume tables and are filtered/joined on these columns in batting/pitching/running stats aggregation and standings recalculation. Missing indexes cause sequential scans as data grows.

Other observations

  • app/db_engine.py had pre-existing ruff/black reformatting in the working tree before this work began (single→double quotes, trailing commas, line wrapping). This noise is included in the diff — consistent with the pattern across previous PRs in this repo.
Closes #74 ## Changes **`app/db_engine.py`** — Added `Meta` classes with `indexes` declarations to `StratGame` and `StratPlay`: - `StratGame.Meta.indexes`: single-column index on `season`; composite index on `(season, week, game_num)` - `StratPlay.Meta.indexes`: single-column indexes on `game` (FK→stratgame), `batter` (FK→player), `pitcher` (FK→player), `runner` (FK→player) **`migrations/2026-03-27_add_stratplay_stratgame_indexes.sql`** — New migration using `CREATE INDEX IF NOT EXISTS` for all 6 indexes. Safe to run on existing databases. ## Why PostgreSQL does not auto-index foreign key columns. `stratplay` and `stratgame` are the highest-volume tables and are filtered/joined on these columns in batting/pitching/running stats aggregation and standings recalculation. Missing indexes cause sequential scans as data grows. ## Other observations - `app/db_engine.py` had pre-existing ruff/black reformatting in the working tree before this work began (single→double quotes, trailing commas, line wrapping). This noise is included in the diff — consistent with the pattern across previous PRs in this repo.
Claude added 1 commit 2026-03-27 10:06:59 +00:00
fix: add missing indexes on FK columns in stratplay and stratgame (#74)
All checks were successful
Build Docker Image / build (pull_request) Successful in 2m2s
Details
2c1d4a3ae0 
Closes #74

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-27 10:15:53 +00:00
Claude reviewed 2026-03-27 10:18:13 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/db_engine.py (modified — Meta.indexes added to StratGame and StratPlay, plus ruff/black noise)
  • migrations/2026-03-27_add_stratplay_stratgame_indexes.sql (added)

Findings

Correctness

  • Peewee Meta.indexes syntax is correct. The format ((("field_name",), is_unique), ...) with False for non-unique indexes is right.
  • FK field names in Meta.indexes are correct. Peewee expects the Python attribute name (e.g. "game", "batter") — not the column name ("game_id") — in Meta.indexes. Peewee's metaclass resolves FK field names to <field>_id column names automatically.
  • Migration SQL uses the correct column names. game_id, batter_id, pitcher_id, runner_id are the actual PostgreSQL column names for FK fields, matching what Peewee generates.
  • ORM declarations match migration SQL exactly. All 6 indexes are declared in both places with consistent table/column names (stratgame, stratplay).
  • No prior Meta on either model. Verified on main — neither StratGame nor StratPlay had a Meta class. The new Meta with only indexes correctly inherits database = db from BaseModel.Meta via Peewee's metaclass.
  • Implicit table names are correct. Peewee defaults StratGame → stratgame and StratPlay → stratplay, matching the migration SQL — no table_name declaration needed for correctness.

Security

  • No security concerns. Pure DDL index addition.

Style & Conventions

  • CREATE INDEX IF NOT EXISTS — idempotent, safe on existing databases. Good practice.
  • Minor suggestion (non-blocking): CLAUDE.md notes models should "always specify Meta.table_name". The new Meta classes omit table_name since the implicit names are correct — functionally fine, but adding table_name = "stratgame" / table_name = "stratplay" would make the convention explicit.

Suggestions

  • The composite index (season, week, game_num) covers the single-column (season) prefix in most query planners. For workloads where the composite index is always present, the standalone season index may be redundant. That said, having the standalone index is harmless and ensures the optimizer uses it for simple WHERE season = X without a covering scan on the composite — reasonable choice.

Verdict: APPROVED

Clean, correct implementation. ORM declarations and migration SQL are aligned. Peewee field-name/column-name convention for FK indexes is correctly handled. IF NOT EXISTS makes the migration safe to replay.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/db_engine.py` (modified — Meta.indexes added to StratGame and StratPlay, plus ruff/black noise) - `migrations/2026-03-27_add_stratplay_stratgame_indexes.sql` (added) ### Findings #### Correctness - **Peewee Meta.indexes syntax is correct.** The format `((("field_name",), is_unique), ...)` with `False` for non-unique indexes is right. - **FK field names in Meta.indexes are correct.** Peewee expects the Python attribute name (e.g. `"game"`, `"batter"`) — not the column name (`"game_id"`) — in `Meta.indexes`. Peewee's metaclass resolves FK field names to `<field>_id` column names automatically. - **Migration SQL uses the correct column names.** `game_id`, `batter_id`, `pitcher_id`, `runner_id` are the actual PostgreSQL column names for FK fields, matching what Peewee generates. - **ORM declarations match migration SQL exactly.** All 6 indexes are declared in both places with consistent table/column names (`stratgame`, `stratplay`). - **No prior Meta on either model.** Verified on main — neither `StratGame` nor `StratPlay` had a `Meta` class. The new `Meta` with only `indexes` correctly inherits `database = db` from `BaseModel.Meta` via Peewee's metaclass. - **Implicit table names are correct.** Peewee defaults `StratGame` → `stratgame` and `StratPlay` → `stratplay`, matching the migration SQL — no `table_name` declaration needed for correctness. #### Security - No security concerns. Pure DDL index addition. #### Style & Conventions - `CREATE INDEX IF NOT EXISTS` — idempotent, safe on existing databases. Good practice. - **Minor suggestion (non-blocking):** CLAUDE.md notes models should "always specify `Meta.table_name`". The new Meta classes omit `table_name` since the implicit names are correct — functionally fine, but adding `table_name = "stratgame"` / `table_name = "stratplay"` would make the convention explicit. #### Suggestions - The composite index `(season, week, game_num)` covers the single-column `(season)` prefix in most query planners. For workloads where the composite index is always present, the standalone `season` index may be redundant. That said, having the standalone index is harmless and ensures the optimizer uses it for simple `WHERE season = X` without a covering scan on the composite — reasonable choice. ### Verdict: APPROVED Clean, correct implementation. ORM declarations and migration SQL are aligned. Peewee field-name/column-name convention for FK indexes is correctly handled. `IF NOT EXISTS` makes the migration safe to replay. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-27 10:18:34 +00:00
All checks were successful
Build Docker Image / build (pull_request) Successful in 2m2s
Details
This pull request has changes conflicting with the target branch.
  • app/db_engine.py
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue/74-add-missing-indexes-on-foreign-key-columns-in-high:issue/74-add-missing-indexes-on-foreign-key-columns-in-high
git checkout issue/74-add-missing-indexes-on-foreign-key-columns-in-high
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working
No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-database#95
No description provided.
Delete Branch "issue/74-add-missing-indexes-on-foreign-key-columns-in-high"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?