Use variant_combinations for query filtering and drop GIN index

[mstaeble]

Summary

• Moves all variant filtering to resolve against the small variant_combinations lookup table (2K rows) first, then filter by integer variant_combination_id, instead of running array containment checks on large tables
• Drops the GIN index on prow_jobs.variants since all filtering now goes through variant_combination_id

Benchmark results (staging)

Query	Before	After	Speedup
TestReportExcludeVariants (@excluded && variants)	1,167ms	55ms	21x
TestsByNURPAndStandardDeviation (= any(variants))	505ms	223ms	2.3x
Collapsed matview filter	1,158ms	500ms	2.3x
PlatformInfraSuccess (unnest + filter)	508ms	346ms	1.5x

Changed functions

• TestsByNURPAndStandardDeviation -- variant_combination_id NOT IN (subquery)
• TestReportsByVariant -- excluded_vc CTE + NOT IN
• TestReportExcludeVariants -- same pattern
• PlatformInfraSuccess -- CTE unnests from variant_combinations, joins matview by integer ID
• buildCollapsedMatViewSQL -- variant_combination_id NOT IN (subquery)
• TestOutputs / TestDurations -- variant_combination_id IN (subquery) instead of GIN-indexed array containment
• GetTestAnalysisOverallFromDB / GetTestAnalysisByJobFromDB -- same pattern

NULL handling note

The old variant exclusion pattern NOT(variants is not null and @excluded && variants) included rows with NULL variants (evaluating to NOT(false) = true). The new variant_combination_id NOT IN (subquery) pattern excludes NULL rows because NULL NOT IN (...) evaluates to NULL (falsy). This is acceptable because:

1. The matview's variant_combination_id comes from test_daily_summaries, which gets it from prow_jobs via the daily summary upsert JOIN
2. The trigger sets variant_combination_id = NULL only when prow_jobs.variants IS NULL
3. On staging, zero matview rows have NULL variant_combination_id -- the variant manager always assigns variants to jobs
4. Rows with truly NULL variants have no meaningful test data to report on

Test plan

• go build ./... and go test ./pkg/db/... ./pkg/api/... pass
• Migration 000004 up/down cycle verified
• Deployed to sippy staging and verified all affected API endpoints return correct data:

#	Endpoint	Function	Result
1	/api/tests (collapsed)	TestsByNURPAndStandardDeviation + collapsed matview	200, 7MB
2	/api/tests (variant filter)	Uncollapsed matview variant filter	200, 8,413 rows
3	/api/tests/details	TestReportsByVariant	200, variant breakdown
4	/api/tests/outputs	TestOutputs	200
5	/api/tests/durations	TestDurations	200
6	/api/variants	VariantReports	200, 69 variants
7	/api/install	PlatformInfraSuccess	200, 2MB
8	/api/tests/analysis/overall	GetTestAnalysisOverallFromDB	200, 12 daily data points
9	/api/tests/analysis/jobs	GetTestAnalysisByJobFromDB	200, 249 job groups

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved variant-based filtering across test analysis, reports, outputs, and duration views for more accurate results.
  • Corrected exclusion handling so selected variants are matched more reliably in queries.
  • Fixed platform infrastructure success calculations to use the updated variant selection logic.
  • Removed reliance on an outdated index and updated database lookups to keep query behavior consistent.

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[mstaeble]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

Multiple SQL query builders and one GORM model tag are updated to filter jobs via prow_jobs.variant_combination_id against the variant_combinations table instead of array checks on prow_jobs.variants. A migration drops the now-unused GIN index on variants, with a corresponding down migration to recreate it.

Changes

Variant filtering migration

Layer / File(s)	Summary
Drop GIN index and update model tag pkg/db/models/prow.go, pkg/db/migrations/000004_drop_variants_gin_index.up.sql, pkg/db/migrations/000004_drop_variants_gin_index.down.sql	Removes the GIN index GORM tag from ProwJob.Variants and adds idempotent up/down migrations to drop/recreate idx_prow_jobs_variants.
Test analysis variant filters pkg/api/test_analysis.go	GetTestAnalysisOverallFromDB and GetTestAnalysisByJobFromDB now filter allowed/blocked variants via IN/NOT IN subqueries against variant_combinations.
Test report and duration query filters pkg/db/query/test_queries.go	TestReportsByVariant, TestReportExcludeVariants, TestsByNURPAndStandardDeviation, TestOutputs, and TestDurations replace array-overlap variant checks with variant_combination_id IN/NOT IN subqueries and CTEs.
Platform infra query and collapsed matview SQL pkg/db/query/misc_queries.go, pkg/db/views.go	PlatformInfraSuccess is rewritten as raw SQL with a target_variants CTE and named parameters; buildCollapsedMatViewSQL builds an ARRAY[...] exclusion list and a single variant_combination_id NOT IN filter.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant PlatformInfraSuccess
  participant DB as Postgres

  Caller->>PlatformInfraSuccess: request platforms + infra test
  PlatformInfraSuccess->>DB: Raw SQL with target_variants CTE (named params)
  DB->>DB: join target_variants to matview, compute pass_percentage
  DB-->>PlatformInfraSuccess: sqlResults rows
  PlatformInfraSuccess-->>Caller: map of variant to pass_percentage

Loading

Related PRs: None found.

Suggested labels: database, sql-migration

Suggested reviewers: None specified.

Poem:
A rabbit hopped through queries deep,
Where variants once did overlap and leap,
Now IDs join with combinations neat,
No GIN index left to compete,
Hop, hop—the schema's tidy and sweet! 🐇

🚥 Pre-merge checks | ✅ 19 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Coverage For New Features	⚠️ Warning	No dedicated regression/unit tests cover the new variant_combination_id filtering; pkg/db/query has no tests, and the existing benchmark cases only smoke-test queries.	Add focused tests for the changed query builders and collapsed matview SQL, asserting variant-filter results/SQL shapes and covering the NULL/exclusion regression.

✅ Passed checks (19 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: moving filtering to variant_combinations and dropping the unused GIN index.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Go Error Handling	✅ Passed	No new error-handling regressions: touched Go paths propagate DB errors, use fmt.Errorf where needed, avoid panic, and guard nil filters/reportEnd.
Sql Injection Prevention	✅ Passed	All user-derived values are parameterized; remaining fmt.Sprintf usage is limited to allowlisted table names/constants, not direct input.
Excessive Css In React Should Use Styles	✅ Passed	PR changes only Go/SQL files; no React components or inline style objects are present, so this CSS style check is not applicable.
Single Responsibility And Clear Naming	✅ Passed	The changes stay tightly scoped: specific query builders, one model tag tweak, and descriptive migration names; no new generic packages or catch-all methods.
Feature Documentation	✅ Passed	docs/features only contains unrelated job-analysis-symptoms.md; no feature doc covers these variant-filtering query changes, and no docs files were changed.
Stable And Deterministic Test Names	✅ Passed	No test files changed, and no Ginkgo test declarations appear in the touched files; the PR only updates queries, models, and migrations.
Test Structure And Quality	✅ Passed	No Ginkgo test files or It/BeforeEach/Eventually usage changed; PR only updates query code and migrations, so the test-structure check is not applicable.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the PR only changes DB/query code and a migration, so there’s nothing to vet for MicroShift APIs.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo/e2e tests were added; the PR only changes DB queries, models, and migrations, so SNO-specific test compatibility is not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	Only DB/API query and migration files changed; no manifests, controllers, or scheduling constraints were introduced, so topology-aware scheduling is not applicable.
Ote Binary Stdout Contract	✅ Passed	Touched files contain only DB/query code; no main/init/TestMain/suite setup or stdout writes were added, and the top-level initializer only builds SQL strings.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo/e2e test code was added; the PR only changes DB/query SQL and migrations, so this compatibility check doesn’t apply.
No-Weak-Crypto	✅ Passed	Touched files only adjust SQL filtering/indexes; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons found in the diff.
Container-Privileges	✅ Passed	PR changes only Go/SQL files; no K8s/container manifests were touched, and no privileged flags or root/host settings appear in the diff.
No-Sensitive-Data-In-Logs	✅ Passed	No new logging was introduced in the diff; existing logs only report counts, errors, or test names, with no passwords/tokens/PII/session data exposed.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

pkg/db/query/misc_queries.go (1)

36-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add inline comments for the new CTE query sections.

target_variants changes how platform matching works by resolving platform names to variant_combination_id; document that intent in the SQL. As per path instructions, “BigQuery and SQL query-building code should have inline comments explaining the purpose of each major query section (CTEs, JOINs, window functions, WHERE clauses).”

Suggested comment

 		WITH target_variants AS (
+			-- Resolve requested platform variants to combination IDs used by the materialized view.
 			SELECT vc.id, v.variant
 			FROM variant_combinations vc, unnest(vc.variants) AS v(variant)
 			WHERE v.variant IN `@platforms`
 		)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/db/query/misc_queries.go` around lines 36 - 50, The new CTE-based SQL in
the query-building code needs inline comments explaining each major section,
especially the target_variants CTE, the JOIN to variant_combinations, and the
filtering/grouping logic. Update the Raw query in the misc query function that
builds the pass-percentage report so the intent of resolving platform names to
variant_combination_id is documented directly in the SQL, with brief comments
for the CTE and join sections.

Source: Path instructions

pkg/db/query/test_queries.go (1)

116-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the new exclusion CTEs.

The new excluded_vc CTEs drive the variant exclusion behavior, but the raw SQL does not explain that section. Add a short inline SQL comment so future query changes preserve the intended variant_combination_id semantics. As per path instructions, “BigQuery and SQL query-building code should have inline comments explaining the purpose of each major query section (CTEs, JOINs, window functions, WHERE clauses).”

Suggested comment

 WITH excluded_vc AS (
+    -- Resolve excluded variant names to combination IDs so rows can be filtered by the lookup key.
     SELECT id FROM variant_combinations WHERE `@excluded` && variants
 ),

Also applies to: 175-193

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/db/query/test_queries.go` around lines 116 - 134, The raw SQL in the
query-building block lacks an explanation for the new excluded-vc CTE and the
variant_combination_id filter. Add a short inline SQL comment in the query
construction around excluded_vc and the NOT IN clause to document that this CTE
defines excluded variant combinations and preserves the intended exclusion
semantics; apply the same comment pattern in the matching query section
referenced by the test helper functions.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/db/query/misc_queries.go`:
- Around line 36-50: The new CTE-based SQL in the query-building code needs
inline comments explaining each major section, especially the target_variants
CTE, the JOIN to variant_combinations, and the filtering/grouping logic. Update
the Raw query in the misc query function that builds the pass-percentage report
so the intent of resolving platform names to variant_combination_id is
documented directly in the SQL, with brief comments for the CTE and join
sections.

In `@pkg/db/query/test_queries.go`:
- Around line 116-134: The raw SQL in the query-building block lacks an
explanation for the new excluded-vc CTE and the variant_combination_id filter.
Add a short inline SQL comment in the query construction around excluded_vc and
the NOT IN clause to document that this CTE defines excluded variant
combinations and preserves the intended exclusion semantics; apply the same
comment pattern in the matching query section referenced by the test helper
functions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: c1df9af7-64f2-427b-bd2a-99842745a7eb

📥 Commits

Reviewing files that changed from the base of the PR and between ef97841 and 5d890c9.

📒 Files selected for processing (8)

• pkg/api/test_analysis.go
• pkg/db/db.go
• pkg/db/migrations/000004_drop_variants_gin_index.down.sql
• pkg/db/migrations/000004_drop_variants_gin_index.up.sql
• pkg/db/models/prow.go
• pkg/db/query/misc_queries.go
• pkg/db/query/test_queries.go
• pkg/db/views.go

[mstaeble]

@coderabbitai resolve

[coderabbitai]

✅ Action performed

Comments resolved and changes approved.

[openshift-merge-bot]

Scheduling required tests:
/test e2e

[petr-muller]

LGTM but needs rebase

[openshift-merge-bot]

Scheduling required tests:
/test e2e

[neisw]

/lgtm

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/db/migrations/000004_drop_variants_gin_index.up.sql`:
- Line 4: The migration currently drops the `idx_prow_jobs_variants` GIN index
in a way that can lock `prow_jobs` too aggressively. Update the standalone
migration to use `DROP INDEX CONCURRENTLY` in the
`000004_drop_variants_gin_index.up.sql` change so the index removal does not
block normal reads and writes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: dfc2360d-33eb-433a-8b20-f3d2395b8cb2

📥 Commits

Reviewing files that changed from the base of the PR and between 5d890c9 and c19fb66.

📒 Files selected for processing (7)

• pkg/api/test_analysis.go
• pkg/db/migrations/000004_drop_variants_gin_index.down.sql
• pkg/db/migrations/000004_drop_variants_gin_index.up.sql
• pkg/db/models/prow.go
• pkg/db/query/misc_queries.go
• pkg/db/query/test_queries.go
• pkg/db/views.go

🚧 Files skipped from review as they are similar to previous changes (5)

• pkg/db/models/prow.go
• pkg/api/test_analysis.go
• pkg/db/query/misc_queries.go
• pkg/db/views.go
• pkg/db/query/test_queries.go

[openshift-merge-bot]

Scheduling required tests:
/test e2e

[neisw]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mstaeble, neisw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [neisw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@mstaeble: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.