Skip to content

chore(trials): aggregate_trials_summary as single CTE-based query#80

Merged
SoundMindsAI merged 2 commits into
mainfrom
chore/trial-summary-single-query
May 13, 2026
Merged

chore(trials): aggregate_trials_summary as single CTE-based query#80
SoundMindsAI merged 2 commits into
mainfrom
chore/trial-summary-single-query

Conversation

@SoundMindsAI
Copy link
Copy Markdown
Owner

Summary

Wave 2 PR A of the pre-MVP2 backlog sweep: SQL refactor per chore_trial_summary_single_query.

backend/app/db/repo/trial.py:aggregate_trials_summary was making two round-trips: one SELECT with COUNT(*) FILTER (...) aggregations + MAX(primary_metric) for counts/best, then a second SELECT to find the trial id matching the best metric. The Phase 2 impl plan committed to this as a single SQL statement.

Rewritten as a CTE (summary) that does the aggregation, joined with a correlated scalar subquery that selects the winner id. One round-trip. Same result.

The 2-query form was functionally correct + well within spec §13's <100ms p99 target on small studies, but documented as single-query in the contract. Closing the drift now while context is loaded.

Test plan

  • pytest backend/tests/unit/ — 696/696 pass
  • mypy backend/app/db/repo/trial.py — clean
  • Existing integration tests (TestTrialsSummary::test_summary_with_mixed_statuses + ::test_summary_empty_study) cover the contract per the original idea spec — no test changes needed
  • CI service container Postgres runs the integration tests against the new query

Archives

chore_trial_summary_single_query/idea.mddocs/00_overview/implemented_features/2026_05_13_chore_trial_summary_single_query/idea.md. Dashboard regenerated.

🤖 Generated with Claude Code

Per chore_trial_summary_single_query: rewrite the helper to use a CTE
that aggregates counts + max(primary_metric), then joins a correlated
scalar subquery for the winner id. Replaces the 2-query implementation
with one round-trip.

Verified: 696 unit tests pass, mypy clean on trial.py. Integration
tests skipped locally (no PG); CI exercises the existing
TestTrialsSummary contract.

Archives chore_trial_summary_single_query to implemented_features.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the aggregate_trials_summary function in backend/app/db/repo/trial.py to use a single CTE-based SQL query, improving performance and adhering to the technical specification. Corresponding updates were made to the project dashboard documentation to reflect the completion of this task. The reviewer suggested using the more idiomatic .filter() syntax for conditional aggregations instead of func.count(case(...)) to improve readability and align with the SQL FILTER clause.

Comment thread backend/app/db/repo/trial.py Outdated
Comment on lines +209 to +212
func.count(case((Trial.status == "complete", 1))).label("complete"),
func.count(case((Trial.status == "failed", 1))).label("failed"),
func.count(case((Trial.status == "pruned", 1))).label("pruned"),
func.max(case((Trial.status == "complete", Trial.primary_metric))).label("best"),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of func.count(case(...)) for conditional aggregation is functional, but SQLAlchemy and modern SQL (like PostgreSQL) offer a more readable and explicit syntax using the .filter() modifier on aggregate functions. This corresponds to the SQL FILTER (WHERE ...) clause.

Using .filter() would make the query's intent clearer and aligns with both the function's docstring and the SQL example in idea.md, which both mention using FILTER.

I suggest updating the conditional aggregations to use this more idiomatic pattern.

Suggested change
func.count(case((Trial.status == "complete", 1))).label("complete"),
func.count(case((Trial.status == "failed", 1))).label("failed"),
func.count(case((Trial.status == "pruned", 1))).label("pruned"),
func.max(case((Trial.status == "complete", Trial.primary_metric))).label("best"),
func.count(Trial.id).filter(Trial.status == "complete").label("complete"),
func.count(Trial.id).filter(Trial.status == "failed").label("failed"),
func.count(Trial.id).filter(Trial.status == "pruned").label("pruned"),
func.max(Trial.primary_metric).filter(Trial.status == "complete").label("best"),

… import

Replaces `func.count(case((cond, 1)))` and `func.max(case((cond, val)))`
with `func.X(val).filter(cond)` — produces SQL `COUNT(*) FILTER (WHERE ...)`
which matches the docstring + the original idea spec's example SQL and
is more idiomatic in PostgreSQL.

The `case` import becomes unused — IDE diagnostic caught it, dropped
from the sqlalchemy import line.

Verified: 696/696 unit tests pass, mypy clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@SoundMindsAI
Copy link
Copy Markdown
Owner Author

Gemini Code Assist review — adjudication

# Finding Severity Verdict Notes
F1 Use func.X(val).filter(cond) idiom instead of func.X(case((cond, val))) Medium Accept (applied in 36b7935) Produces SQL COUNT(*) FILTER (WHERE ...) which matches the docstring + the original idea spec's SQL example. More idiomatic in PostgreSQL. Per the new rubric: same file, ~4 LOC, no test changes → inline. IDE diagnostic caught case import becoming unused; dropped from the sqlalchemy import. Verified: 696/696 unit tests pass, mypy clean.

Net: 1 finding → 1 accept (applied).

🤖 Generated with Claude Code

@SoundMindsAI SoundMindsAI merged commit 1067dd8 into main May 13, 2026
4 checks passed
@SoundMindsAI SoundMindsAI deleted the chore/trial-summary-single-query branch May 13, 2026 14:52
SoundMindsAI added a commit that referenced this pull request May 13, 2026
…skill (#81)

Records the 2026-05-13 backlog sweep + new skill in state.md per
the standard "update state.md whenever a feature lands or a priority
shifts" rule:

- Updates "Last updated" line to reflect the sweep
- Adds 4 entries to "Most recent meaningful changes":
  - the 9-item backlog sweep (PRs #75-#80)
  - the /bug-fix skill (PRs #71-#72) + Investigation-mode dogfood on
    bug_chat_long_conversation_truncation (PR #73)
  - the CLAUDE.md inline-fix vs idea-file rubric (PR #74)
  - the docs-only paths-ignore CI filter (PR #70)
- Expands "Active feature" line with the remaining-backlog breakdown
  (6 inline + 3 /bug-fix + 7 needs-second-pass + 4 keep-deferred)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SoundMindsAI added a commit that referenced this pull request Jun 3, 2026
…corecard alerts) (#431)

* ci(security): add CodeQL SAST + write Dockerfile base-image digests literally

Two follow-ups on the post-merge OSSF Scorecard surface (PR #430 closed the
postcss vuln + ~50 pinning alerts; this closes the SAST finding + the 6
remaining containerImage alerts).

CodeQL (#71): add .github/workflows/codeql.yml — GitHub first-party SAST on
push/PR to main + weekly, scanning python + javascript-typescript with
build-mode none. Closes the Scorecard "SAST tool is not run on all commits"
finding and surfaces real code-level bugs on every PR. Action refs pinned to
SHAs to match the repo's pinning posture.

Dockerfile digests (#59/#60/#80-#83): revert the BASE_IMAGE ARG indirection
back to literal `image@sha256:…` on each FROM. The pin was always real, but
Scorecard's static parser only credits a digest it can see inline — an
ARG-indirected digest reads as "unpinned", which is what kept all 6
containerImage alerts open (Dockerfile:24/46/79 + ui/Dockerfile:24/30/39,
confirmed from the Scorecard SARIF locations). Writing tag+digest together also
removes the override footgun Gemini flagged on PR #430; Dependabot bumps every
FROM occurrence in lockstep, so the repeated node digest is not a sync hazard.
Both Dockerfiles pass `docker buildx build --check`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>

* ci(security): builder stage inherits from deps (Gemini #431)

Accept Gemini's finding on PR #431: have the ui builder stage `FROM deps`
instead of re-deriving from the base image. deps already has pnpm, WORKDIR,
and node_modules, so this drops a redundant `npm install -g pnpm@9` + the
node_modules COPY, removes one base-image digest occurrence, and eliminates one
of the two npmCommand Scorecard findings. A stage ref to a digest-pinned stage
is still credited as pinned by Scorecard. node_modules is .dockerignore'd so
`COPY . .` doesn't clobber the installed deps. Verified with a full
`docker buildx build` of the ui image (next build + runner copy succeed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>

---------

Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant