Skip to content

drop unused array_agg columns from job runs matview#3652

Merged
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
mstaeble:drop-unused-test-ids-from-job-runs-matview
Jun 23, 2026
Merged

drop unused array_agg columns from job runs matview#3652
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
mstaeble:drop-unused-test-ids-from-job-runs-matview

Conversation

@mstaeble

@mstaeble mstaeble commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Remove failed_test_ids and flaked_test_ids array_agg calls from the test_results CTE in prow_job_runs_report_matview. These columns were computed inside the CTE but never selected by the outer query — they have been dead code since the matview was introduced in PR #365 (2022).

Context

The test_results CTE aggregates failed/flaked test data from prow_job_run_tests. It computed four array_agg columns but only two (failed_test_names, flaked_test_names) were ever referenced in the outer SELECT. The ID variants (failed_test_ids, flaked_test_ids) added unnecessary work during matview refresh — they inflated the sort data volume without contributing to the materialized result.

Staging benchmark (CTE portion only): 755s → 614s execution time (~19% faster).

Test plan

  • go build ./...
  • go vet ./...
  • go test ./pkg/db/... ./pkg/api/... ./pkg/filter/...
  • Verify matview refresh completes successfully after deploy
  • Verify /api/jobs/runs returns correct data (no schema change to API consumers)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Updated the database view used for job run reporting so test results are aggregated more precisely.
    • The report now focuses on failed and flaked test counts and names for status codes 12 and 13, omitting additional aggregated test ID fields.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

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

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Updates pkg/db/views.go to modify the jobRunsReportMatView SQL's test_results CTE by removing aggregation of failed_test_ids and flaked_test_ids, replacing it with aggregations of only failed_test_count/failed_test_names and flaked_test_count/flaked_test_names for status codes 12 and 13.

Changes

jobRunsReportMatView SQL Update

Layer / File(s) Summary
test_results CTE aggregation refactor
pkg/db/views.go
Removes failed_test_ids and flaked_test_ids array aggregations from the test_results CTE and replaces them with count and name-based aggregations for failed and flaked test status codes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • stbenjam
  • xueqzhan
🚥 Pre-merge checks | ✅ 20 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Go Error Handling ⚠️ Warning The new pkg/db/views.go file contains three instances of uncontextualized error returns (lines 111, 119, 149) that violate the requirement to wrap errors with context using fmt.Errorf with %w. Wrap the returned errors with context: return fmt.Errorf("syncing [view/index name]: %w", err) at lines 111, 119, and 149 in pkg/db/views.go.
✅ Passed checks (20 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically describes the main change: removing unused array aggregation columns (failed_test_ids and flaked_test_ids) from the job runs materialized view.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Sql Injection Prevention ✅ Passed PR removes two unused array_agg columns from a SQL CTE constant; no new SQL concatenation, user input handling, or parameterization issues introduced.
Excessive Css In React Should Use Styles ✅ Passed Custom check for excessive CSS in React components is not applicable. PR modifies only pkg/db/views.go (Go source), not React components or JavaScript files containing CSS styling.
Test Coverage For New Features ✅ Passed This PR removes unused SQL columns from a CTE within a materialized view definition string. No new Go functions or methods are introduced. The change is a refactoring covered by existing benchmark...
Single Responsibility And Clear Naming ✅ Passed PR only modifies SQL string content, not package/struct/method definitions. No naming or responsibility violations introduced; check not applicable to constant string modifications.
Feature Documentation ✅ Passed The PR removes dead code (unused CTE columns) from a matview—a pure internal optimization with no API schema, data model, or data flow changes. No feature documentation updates are required.
Stable And Deterministic Test Names ✅ Passed Repository uses standard Go testing (not Ginkgo), all test names are static and deterministic with no dynamic content.
Test Structure And Quality ✅ Passed PR only modifies SQL view definitions in pkg/db/views.go; contains no Ginkgo test code or test file changes, making this test-focused check not applicable.
Microshift Test Compatibility ✅ Passed No Ginkgo e2e tests are added in this PR. The changes only modify SQL view definitions in pkg/db/views.go (a database schema change), not e2e test code.
Single Node Openshift (Sno) Test Compatibility ✅ Passed This PR does not add any Ginkgo e2e tests; it only modifies SQL view definitions in pkg/db/views.go. The SNO test compatibility check is not applicable.
Topology-Aware Scheduling Compatibility ✅ Passed This PR modifies only database code (SQL views in pkg/db/views.go) with no Kubernetes manifests, operator code, controllers, or scheduling constraints. The topology-aware scheduling check is not ap...
Ote Binary Stdout Contract ✅ Passed PR modifies only SQL query constants in pkg/db/views.go; contains no stdout writes, klog config, or process-level code that could violate the OTE binary stdout contract.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed PR modifies SQL view definitions in pkg/db/views.go, not Ginkgo e2e tests. Check only applies to new e2e test additions; not applicable here.
No-Weak-Crypto ✅ Passed PR contains only SQL optimization changes to views.go (removing unused array aggregations and replacing CURRENT_TIMESTAMP). No weak cryptography (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom c...
Container-Privileges ✅ Passed PR modifies only pkg/db/views.go SQL logic; contains no container or Kubernetes manifests, so the container-privileges check is not applicable.
No-Sensitive-Data-In-Logs ✅ Passed This PR only modifies SQL view definitions to remove unused columns and optimize matview performance; no logging code is added or modified that could expose sensitive data.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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

Comment @coderabbitai help to get the list of available commands and usage tips.

@mstaeble mstaeble changed the title Drop unused array_agg columns from job runs matview [WIP] Drop unused array_agg columns from job runs matview Jun 18, 2026
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2026
@openshift-ci

openshift-ci Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

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 openshift-ci Bot added the ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review label Jun 18, 2026
Remove failed_test_ids and flaked_test_ids array_agg calls from the
test_results CTE in prow_job_runs_report_matview. These columns were
computed inside the CTE but never selected by the outer query — they
have been dead code since the matview was introduced in PR openshift#365 (2022).

Removing them reduces the sort data volume during matview refresh.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mstaeble mstaeble force-pushed the drop-unused-test-ids-from-job-runs-matview branch from 6b1cabe to 8c47faa Compare June 18, 2026 18:49
@mstaeble mstaeble changed the title [WIP] Drop unused array_agg columns from job runs matview drop unused array_agg columns from job runs matview Jun 18, 2026
@mstaeble mstaeble marked this pull request as ready for review June 18, 2026 19:45
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2026
@openshift-ci openshift-ci Bot requested review from smg247 and sosiouxme June 18, 2026 19:45
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2026
@openshift-ci

openshift-ci Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mstaeble, petr-muller

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:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2026
@mstaeble

Copy link
Copy Markdown
Contributor Author

/retest ci/prow/unit

@mstaeble

Copy link
Copy Markdown
Contributor Author

/test ci/prow/unit

@mstaeble

Copy link
Copy Markdown
Contributor Author

/test unit

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e

@openshift-ci

openshift-ci Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

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

@openshift-merge-bot openshift-merge-bot Bot merged commit 84be412 into openshift:main Jun 23, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants