-
Notifications
You must be signed in to change notification settings - Fork 29
perf: add database index #131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis PR adds comprehensive database indexing across three models (Kind, SharedTeam, Subtask) by declaring column-level and composite table-level indexes. A new verification script validates that all indexes are properly created in the database. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Areas requiring attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/app/models/shared_team.py (1)
19-27: SharedTeam index set looks solid; verify redundancy vs. actual query patternsAdding
index=Trueonis_activeplusidx_user_active(user_id, is_active)andidx_team_active(team_id, is_active)should help lookups of active shared teams by user or team. Sinceuser_id,team_id, andis_activealso have single-column indexes and there is alreadyidx_user_team, it’s worth checking real query plans/slow logs to confirm each index is pulling its weight and trimming any that end up unused to avoid extra write and storage cost over time.backend/app/models/subtask.py (1)
7-60: Subtask indexing strategy is reasonable; keep an eye on overlap and index bloatIndexing
user_id,task_id,team_id, executor fields,status, andcreated_atplus the composite subtasks indexes (idx_subtask_task_user,..._task_status,..._task_role_status,..._task_message,idx_subtask_executor) lines up well with typical filtering patterns. It is, however, a dense set of overlapping indexes (e.g.,task_idandstatusboth indexed alone and together), so after this ships it’s worth checking EXPLAIN plans/slow-query logs to confirm which combinations are actually used and consider dropping any clearly redundant single-column indexes to keep insert/update cost and index size under control.backend/scripts/verify_indexes.py (2)
19-103: Index verification logic is clear; consider documenting or extending coverageThe inspector-based checks for the named composite indexes on
kinds,subtasks, andshared_teamsare straightforward and the output is easy to read. Right now the script only asserts the presence of those explicitly named indexes; it doesn’t verify the single-column indexes created viaindex=Trueon the models (e.g.,user_id,is_active,status,created_at). That’s fine if the intent is “sanity-check composite indexes only”, but it may be worth either (a) documenting this scope in the module docstring/help text or (b) later extending the script to also validate critical single-column indexes so schema drift in migrations gets caught early.
104-111: BroadExceptioncatch at entrypoint (BLE001) – either narrow or explicitly allowCatching
Exceptionin the__main__block is a pragmatic way to ensure a clean non-zero exit with a traceback if anything goes wrong, but Ruff flags this as BLE001. If you want to keep the broad catch (which is reasonable for a CLI), consider adding an inline comment and# noqa: BLE001to make the intent explicit, or alternatively narrow it to the concrete exceptions you expect from SQLAlchemy/DB connection failures so the linter stays happy without a suppression.backend/app/models/kind.py (1)
8-32: Kind indexes are well targeted; monitor overlap across user/kind/name/namespaceIndexing
user_id,is_active, andcreated_atplus the four composite indexes (idx_kind_user_kind,..._user_kind_active,..._user_name_namespace,..._user_kind_name_namespace) should materially speed up the common queries over kinds by user, kind, name, namespace, and active status. As with the Subtask/SharedTeam models, this is a fairly rich index set with some overlap, so after deployment it’s worth watching write/query performance and pruning any indexes that turn out unused or redundant to keep the table lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/models/kind.py(2 hunks)backend/app/models/shared_team.py(1 hunks)backend/app/models/subtask.py(2 hunks)backend/scripts/verify_indexes.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/scripts/verify_indexes.py (3)
backend/app/models/kind.py (1)
Kind(14-34)backend/app/models/subtask.py (1)
Subtask(26-60)backend/app/models/shared_team.py (1)
SharedTeam(11-28)
🪛 Ruff (0.14.5)
backend/scripts/verify_indexes.py
107-107: Do not catch blind exception: Exception
(BLE001)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.