perf: push filters into test report subqueries (~830x improvement)#3290
perf: push filters into test report subqueries (~830x improvement)#3290stbenjam wants to merge 2 commits intoopenshift:mainfrom
Conversation
When collapse=false, TestsByNURPAndStandardDeviation builds a query that self-joins prow_test_report_7d_matview 3 times: 1. Outer query - gets the raw rows 2. pass_rates subquery - computes per-variant percentages 3. stats subquery - computes AVG/STDDEV across variants The name/variant filters were only applied to the outermost query. Subqueries 2 and 3 scanned all rows for the release to compute aggregates for every test, even when only a single test was requested. For release 4.22 with a name filter, this meant: | | Before (outer only) | After (pushed down) | |--------------------|-------------------------|---------------------| | Stats subquery | Seq Scan, 1.28M rows | Index Scan, 142 | | Estimated cost | 802,603 - 1,137,530 | 7.53 - 1,371 | | Speedup | - | ~830x | TestsByNURPAndStandardDeviation now accepts optional filter functions (variadic, backward-compatible) that are applied to both the stats and pass_rates subqueries. The filter is still also applied to the outer query, so results are identical. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (2)
WalkthroughAdds a SubqueryFilter type and propagates optional subquery filters from the API into TestsByNURPAndStandardDeviation, which now accepts variadic filters and applies them (via funcs taking *gorm.DB) to stats and passRates subqueries before joining and aggregating. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stbenjam 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/db/query/test_queries.go`:
- Around line 267-272: Applying all subqueryFilters to both stats and passRates
can alter stats semantics (delta/stddev); split filters so variant-specific
conditions only affect passRates. Update the loop over subqueryFilters to
classify each filter (e.g., via a new helper isVariantFilter(f) or a method on
the filter) and then apply: if isVariantFilter(f) { passRates = f(passRates) }
else { stats = f(stats); passRates = f(passRates) }; keep names subqueryFilters,
stats and passRates to locate the change and add the isVariantFilter helper
implementation to detect filters that reference variant-specific fields.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
pkg/api/tests.gopkg/db/query/test_queries.go
Variant-specific filters (e.g., NOT has entry "never-stable") must not be pushed into the stats subquery, which computes AVG/STDDEV across all variants for a test. Filtering out variants there would skew the delta_from_*_average and standard deviation calculations. Split SubqueryFilter into a struct with a VariantOnly flag and an isVariantFilter helper. At the call site, the rawFilter is further split: name filters go to both stats and passRates subqueries (safe, just narrows to the matching test), while variant filters go only to passRates (preserving cross-variant stats semantics). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Scheduling required tests: |
|
/test e2e |
|
@stbenjam: The following test failed, say
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. |
2 participants
When collapse=false, TestsByNURPAndStandardDeviation builds a query that self-joins prow_test_report_7d_matview 3 times:
The name/variant filters were only applied to the outermost query. Subqueries 2 and 3 scanned all rows for the release to compute aggregates for every test, even when only a single test was requested.
For release 4.22 with a name filter, this meant:
TestsByNURPAndStandardDeviation now accepts optional filter functions (variadic, backward-compatible) that are applied to both the stats and pass_rates subqueries. The filter is still also applied to the outer query, so results are identical.
Summary by CodeRabbit
Chores
Bug Fixes