Combined CR query with materialized CTEs - #3884
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe component readiness report now retrieves base and sample statuses through one provider API. Providers return both result maps and aggregated errors. Middleware queries use a shared wait group and error channel. PostgreSQL combines status queries, and integration coverage expands. ChangesComponent readiness status flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GenerateReport
participant MiddlewareList
participant TestStatusQuerier
participant PostgresProvider
participant PostgreSQL
GenerateReport->>TestStatusQuerier: QueryTestStatus(ctx, requestOptions)
GenerateReport->>MiddlewareList: Query(ctx, waitGroup, errorChannel)
MiddlewareList-->>GenerateReport: Middleware errors
TestStatusQuerier->>PostgresProvider: QueryTestStatus(ctx, requestOptions)
PostgresProvider->>PostgreSQL: Execute combined base and sample query
PostgreSQL-->>PostgresProvider: Source-tagged status rows
PostgresProvider-->>TestStatusQuerier: Base and sample status maps
TestStatusQuerier-->>GenerateReport: Status maps and query errors
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 17 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (17 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/api/componentreadiness/dataprovider/postgres/cr_queries.go (4)
434-451: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
prepareVariantQueryinstead of re-implementing it per side.Lines 416-451 repeat the body of
prepareVariantQuerytwice:lookupVariantValues,buildVariantFilterClause, and theSELECT vc.id FROM variant_combinations vc [WHERE ...]assembly.prowJobJoinTemplatealso duplicates the join string thatqueryTestStatusbuilds inline at lines 246-250. Any future change to the variant filter or the prow job join must be applied in two places.Extract the per-side setup into a helper that both
queryTestStatusandqueryCombinedTestStatuscall, and promote the prow job join to a package-level constant.♻️ Proposed shape
const prowJobJoinTemplate = `JOIN prow_jobs pj ON pj.id = e.prow_job_id AND pj.deleted_at IS NULL AND pj.variant_combination_id IN (%s) JOIN vg ON vg.vcid = pj.variant_combination_id` // variantSide holds the per-side variant resolution used by the combined query. type variantSide struct { lookup map[uint]map[string]string filterArgs []any prowJobJoin string } func resolveVariantSide(ctx context.Context, dbc *db.DB, includeVariants map[string][]string, dbGroupBy sets.Set[string]) (variantSide, error) { lookup, err := lookupVariantValues(ctx, dbc, includeVariants, dbGroupBy) if err != nil { return variantSide{}, err } filterClause, filterArgs := buildVariantFilterClause(includeVariants) subquery := "SELECT vc.id FROM variant_combinations vc" if filterClause != "" { subquery += " WHERE " + filterClause } return variantSide{ lookup: lookup, filterArgs: filterArgs, prowJobJoin: fmt.Sprintf(prowJobJoinTemplate, subquery), }, nil }🤖 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/api/componentreadiness/dataprovider/postgres/cr_queries.go` around lines 434 - 451, Extract the repeated variant-side setup into a shared resolveVariantSide helper and package-level prowJobJoinTemplate. Update both queryTestStatus and queryCombinedTestStatus to use the helper for lookup values, filter arguments, subquery construction, and Prow job joins, preserving existing error propagation and query behavior.
555-568: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the side as a log field, not as part of the message.
mergePlaceholdersformatslabelinto the message withInfofwhile the counts useWithField. This makes the log line hard to filter by side. Use a field for the side and a constant message.♻️ Proposed change
- log.WithField("placeholders", len(placeholders)). + log.WithField("side", label). + WithField("placeholders", len(placeholders)). WithField("merged", merged). WithField("failures", len(failures)-merged). WithField("total", len(failures)). - Infof("combined query: %s placeholder merge complete", label) + Info("combined query: placeholder merge complete")As per coding guidelines: "Prefer structured logging where appropriate, especially for names and IDs, and prefer
log.WithField()over formatting values into strings."🤖 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/api/componentreadiness/dataprovider/postgres/cr_queries.go` around lines 555 - 568, Update mergePlaceholders to add label as a structured log field via WithField, and replace the Infof call with a constant message using Info. Keep the existing placeholder, merged, failures, and total fields unchanged.Source: Coding guidelines
501-536: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftStream the combined rows instead of materializing the full union.
tx.Raw(...).Scan(&allRows)builds the complete result slice before the split loop runs. The union now carries both sides, so peak memory holds every sample row plus every base row ascombinedRowvalues, in addition to the four result maps. The comment inpkg/api/componentreadiness/component_report.goat line 318 records a production base result count of 133132 rows, so the combined slice can reach a few hundred thousand structs, each holding several strings and apq.StringArray.
scanRowsat line 596 already streams with.Rows()and inserts directly into the map. Use the same approach here and add thesourcecolumn, so the intermediate slice disappears and the row-to-TestStatusconversion is defined once.♻️ Proposed streaming shape
- var allRows []combinedRow - if qErr := tx.Raw(fullSQL, allArgs...).Scan(&allRows).Error; qErr != nil { - return fmt.Errorf("querying combined test status: %w", qErr) - } - sampleFailures := make(map[string]crstatus.TestStatus) samplePlaceholders := make(map[string]crstatus.TestStatus) baseFailures := make(map[string]crstatus.TestStatus) basePlaceholders := make(map[string]crstatus.TestStatus) scanStart := time.Now() - for _, row := range allRows { + rows, qErr := tx.Raw(fullSQL, allArgs...).Rows() + if qErr != nil { + return fmt.Errorf("querying combined test status: %w", qErr) + } + defer rows.Close() + + rowCount := 0 + for rows.Next() { + var row combinedRow + if err := rows.Scan( + &row.Source, &row.TestID, &row.TestName, &row.TestSuite, + &row.Component, &row.Capabilities, &row.VariantGroupID, + &row.TotalCount, &row.SuccessCount, &row.FlakeCount, &row.LastFailure, + ); err != nil { + return fmt.Errorf("scanning combined row: %w", err) + } + rowCount++ variantMap := groupMapping.groupToVariants[row.VariantGroupID]Then check
rows.Err()after the loop and logrowCountinstead oflen(allRows).🤖 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/api/componentreadiness/dataprovider/postgres/cr_queries.go` around lines 501 - 536, Replace the `tx.Raw(fullSQL, allArgs...).Scan(&allRows)` materialization in the combined-row processing flow with `Rows()`, selecting the `source` column and scanning each row into `combinedRow` as it streams. Reuse one row-to-`TestStatus` conversion path while inserting directly into the four maps, close the rows, check `rows.Err()`, and track/log a `rowCount` instead of using `len(allRows)`; follow the existing `scanRows` pattern.
34-35: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid disabling PostgreSQL planner methods globally.
enable_sort = offandenable_nestloop = offare planner diagnostics; PostgreSQL still uses sort or nested-loop paths when they are the only viable option, but with a different cost/selection model. These hints apply to the whole transaction, so prefer targeting only the queries that need them and documentEXPLAIN (ANALYZE, BUFFERS)results for both versions.🤖 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/api/componentreadiness/dataprovider/postgres/cr_queries.go` around lines 34 - 35, Update queryPlannerHints to remove the global enable_sort and enable_nestloop planner overrides, and apply any needed planner settings only to the specific queries that require them. Validate the affected queries with EXPLAIN (ANALYZE, BUFFERS) both with and without those settings, and document the comparison.
🤖 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/api/componentreadiness/dataprovider/postgres/cr_queries.go`:
- Around line 416-426: Replace the combined len(sampleLookup) || len(baseLookup)
early return in the surrounding query function with independent handling for
sampleLookup and baseLookup. Preserve non-empty results for either side,
returning an empty sample or base map only when that side’s own lookup is empty,
and reuse the already-resolved sampleRange without resolving it again.
In `@test/integration/component_readiness_test.go`:
- Around line 3764-3767: Update the assertions for the shared component rows in
the test cases around findReportRow and findReportColumn, including the
duplicate case near the “not MissingSample/MissingBasis” comment, to explicitly
reject both crtest.MissingBasis and crtest.MissingSample. Replace the current
lower-bound assertion with checks that the status is neither missing condition,
preserving the intent that shared data is assessed normally.
---
Nitpick comments:
In `@pkg/api/componentreadiness/dataprovider/postgres/cr_queries.go`:
- Around line 434-451: Extract the repeated variant-side setup into a shared
resolveVariantSide helper and package-level prowJobJoinTemplate. Update both
queryTestStatus and queryCombinedTestStatus to use the helper for lookup values,
filter arguments, subquery construction, and Prow job joins, preserving existing
error propagation and query behavior.
- Around line 555-568: Update mergePlaceholders to add label as a structured log
field via WithField, and replace the Infof call with a constant message using
Info. Keep the existing placeholder, merged, failures, and total fields
unchanged.
- Around line 501-536: Replace the `tx.Raw(fullSQL, allArgs...).Scan(&allRows)`
materialization in the combined-row processing flow with `Rows()`, selecting the
`source` column and scanning each row into `combinedRow` as it streams. Reuse
one row-to-`TestStatus` conversion path while inserting directly into the four
maps, close the rows, check `rows.Err()`, and track/log a `rowCount` instead of
using `len(allRows)`; follow the existing `scanRows` pattern.
- Around line 34-35: Update queryPlannerHints to remove the global enable_sort
and enable_nestloop planner overrides, and apply any needed planner settings
only to the specific queries that require them. Validate the affected queries
with EXPLAIN (ANALYZE, BUFFERS) both with and without those settings, and
document the comparison.
🪄 Autofix
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: Pro Plus
Run ID: 00dffccd-b446-4f47-92e1-1abfd96aaf8d
📒 Files selected for processing (13)
pkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/dataprovider/mixed/provider.gopkg/api/componentreadiness/dataprovider/postgres/cr_queries.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/middleware/interface.gopkg/api/componentreadiness/middleware/linkinjector/linkinjector.gopkg/api/componentreadiness/middleware/list.gopkg/api/componentreadiness/middleware/regressionallowances/regressionallowances.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gotest/integration/component_readiness_test.go
0703b9a to
ee2c819
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Exercise the full GenerateReport pipeline (combined query path) with 9 test scenarios: no regression, regression detection, cross-release isolation, missing sample/basis, variant grouping collapse, cross-variant compare, GA base path, lifecycle filtering, and minimum failure threshold. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fold the separate sample and base queries into a single SQL statement with two materialized CTEs (sample_agg, base_agg) joined via UNION ALL. This eliminates the concurrent partition scans that cause buffer cache contention when sample and base queries run in parallel. The postgres provider now implements CombinedTestStatusQuerier, which GenerateReport prefers over the separate QueryBase/QuerySample path. Cross-variant compare, GA base windows, lifecycle filtering, and drilldown filters are all supported in the combined path. Also fixes a bug where the sample CTE unconditionally applied a lifecycle filter (AND e.lifecycle = ANY(?)), causing zero sample results when no lifecycle was specified. The filter is now conditional, matching the behavior of the separate query path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ee2c819 to
5fdcc5e
Compare
|
Scheduling required tests: |
|
@mstaeble: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
1 participant
Summary
component_readinessqueries into a single SQL statement using two materialized CTEs and UNION ALL branches, eliminating duplicate query planning and table scans.getTestStatusorchestration: the combined query returns both sides in one call, removing goroutine/channel coordination for the common case.GenerateReportintegration tests covering prefix-sum aggregation, GA path, mixed lifecycle filtering, disjoint variants between base and sample, capabilities array-overlap filtering, and grid placeholder merging.Benchmark results
Tested locally against the staging database (3 runs each, averaged, with warm-up):
The combined query provides ~2x speedup on large views and up to 2.6x on cross-variant views. Small views (ROSA, Hypershift) with fewer variant groups show no meaningful change. The test_details drilldown path (which still uses the standalone query) is not regressed.
Test plan
make integration)GenerateReport_*andTestCapabilitiesArrayOverlapFilterintegration tests passtest_detailsdrill-down path still works (standalone query path)QueryBaseTestStatus)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance
Observability