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
  • Variant-based filtering now behaves more consistently across test analysis, reports, outputs, and duration views.
  • Excluded variants are handled more reliably, improving the accuracy of results shown for selected test variants.
  • Platform infrastructure success metrics now use the updated filtering approach for more consistent reporting.

[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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mstaeble
Once this PR has been reviewed and has the lgtm label, please assign stbenjam for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[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

Variant filtering now resolves through variant_combination_id lookups against variant_combinations across analysis, report, platform-infra, and collapsed mat-view SQL. The prow_jobs.variants GIN index is dropped and the model tag is updated to match.

Changes

Variant combination filtering

Layer / File(s)	Summary
Index and model metadata 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, pkg/db/db.go	The ProwJob.Variants tag removes the GIN index declaration, the migration drops and recreates idx_prow_jobs_variants, and the trigger comment describes truncating test_daily_summaries when variant_combination_id is missing.
Analysis and report query filters pkg/api/test_analysis.go, pkg/db/query/test_queries.go	GetTestAnalysisOverallFromDB, GetTestAnalysisByJobFromDB, and the report query builders replace direct prow_jobs.variants checks with variant_combination_id lookups and variant_combinations-backed exclusion CTEs.
Platform infra success query pkg/db/query/misc_queries.go	PlatformInfraSuccess switches to a raw SQL query with target_variants, named parameters, and sqlResults scanning for pass_percentage.
Collapsed mat view exclusion SQL pkg/db/views.go	buildCollapsedMatViewSQL() now builds an excluded-variant array and filters on variant_combination_id NOT IN against variant_combinations.

Sequence Diagram(s)

sequenceDiagram
  participant GetTestAnalysisOverallFromDB
  participant variant_combinations
  participant prow_jobs

  GetTestAnalysisOverallFromDB->>variant_combinations: resolve requested variants to ids
  variant_combinations-->>GetTestAnalysisOverallFromDB: matching variant_combination_id values
  GetTestAnalysisOverallFromDB->>prow_jobs: filter rows by variant_combination_id IN / NOT IN

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• openshift/sippy#3678: Introduces variant_combination_id usage in matviews and summaries, which this PR extends into query filtering.

Suggested reviewers

• deepsm007
• stbenjam
• sosiouxme
• petr-muller

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error, 2 warnings)

Check name	Status	Explanation	Resolution
Container-Privileges	❌ Error	FAIL: the top-level Dockerfile’s runtime stage has no USER directive, so it defaults to root with no justification; no other privileged flags were found.	Set a non-root USER for the runtime image, or document why root is required, and keep privileged/hostNetwork/allowPrivilegeEscalation flags absent.
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	Several changed query builders and the pure buildCollapsedMatViewSQL helper have no direct regression/unit tests; existing tests only cover unrelated view refresh ordering.	Add unit/regression tests for the altered query builders and SQL generator (or extract testable pure helpers) to lock in variant_combination_id filtering.

✅ Passed checks (18 passed)

Check name	Status	Explanation
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	Changed Go paths propagate DB errors, use fmt.Errorf where needed, avoid panic/_ suppressions, and guard filters before dereferencing.
Sql Injection Prevention	✅ Passed	All changed SQL paths use placeholders or fixed identifiers; the only dynamic table names come from validated constants, not user input.
Excessive Css In React Should Use Styles	✅ Passed	PR only changes Go/SQL backend files; no React components or inline style objects were touched, so the CSS rule is not applicable.
Single Responsibility And Clear Naming	✅ Passed	The PR keeps query functions focused on variant filtering, uses descriptive names like variant_combination_id/target_variants, and adds no new broad structs or unclear APIs.
Feature Documentation	✅ Passed	Only docs/features/job-analysis-symptoms.md exists, and it covers symptoms/labels, not variant filtering or test-analysis paths, so no feature doc update was needed.
Stable And Deterministic Test Names	✅ Passed	Touched files are query/model/migration code only; no Ginkgo titles or dynamic test names were added or changed.
Test Structure And Quality	✅ Passed	No Ginkgo test files were changed in the PR; the touched files are SQL/docs/model/query code only, so the test-structure check is not applicable.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e specs were added; the changed files are query/model/migration code and contain no It/Describe/Context/When blocks.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the patch only changes DB/query/migration code and contains no SNO-relevant test bodies.
Topology-Aware Scheduling Compatibility	✅ Passed	PR only changes DB/API SQL and migrations; no manifests, controllers, replicas, affinity, node selectors, PDBs, or topology-based scheduling logic.
Ote Binary Stdout Contract	✅ Passed	Touched files only alter SQL/comments/model tags; no main/init/TestMain suite setup or stdout writes (fmt.Print/klog/os.Stdout) were added.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No Ginkgo e2e tests were added; the PR only changes backend SQL/query code and migrations, with no IPv4 or external-network test code.
No-Weak-Crypto	✅ Passed	Changed files only add SQL/filtering logic; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons. sha256 is only for schema hashes.
No-Sensitive-Data-In-Logs	✅ Passed	The PR only changes query filters/comments; no new logs expose passwords, tokens, PII, hostnames, or customer data, and existing logs are count/error-only.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: switching filtering to variant_combinations and removing the GIN index.

✨ 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

[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.