fix(metrics): build the bucket series in FROM, not the select list - #433
fix(metrics): build the bucket series in FROM, not the select list#433vyruss wants to merge 1 commit into
Conversation
The gap-filling `all_buckets` CTE in the metrics time-series queries called `generate_series` from the select list. A set-returning function in that position has no DuckDB equivalent: DuckDB's timestamp `generate_series` returns a LIST there rather than a set, so reading the `metrics.*` tables through a view executed by DuckDB fails with "Unimplemented type for cast (TIMESTAMP WITH TIME ZONE -> TIMESTAMP WITH TIME ZONE[])". Move the call into the `FROM` clause at both sites, in `BuildMetricsQuery` and `BuildDerivedMetricsQuery`. The two forms return identical rows in PostgreSQL, so the rewrite is backend-agnostic and needs no conditional. The derived-query test asserted only the `generate_series(...)` substring, which both forms contain; it now pins the full `FROM generate_series(...) AS g(bucket_time)` form, and `TestBuildMetricsQuery` gains the same assertion so neither site can regress.
b3b420b to
38557ae
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughBoth metrics query builders now generate bucket timestamps through a ChangesMetrics query generation
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to The change moves bucket-series construction into the SQL FROM clause to support DuckDB while preserving PostgreSQL results; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
1 participant
Summary
The gap-filling bucket series in the metrics time-series queries is
built with
generate_seriesin the select list. This moves it into theFROMclause. The two forms return identical rows in PostgreSQL, sothis is a pure rewrite of the query text with no behaviour change.
The reason for the change is that a set-returning function in a select
list has no equivalent in DuckDB. When the
metrics.*tables are readthrough a view whose query is executed by DuckDB rather than
PostgreSQL, DuckDB's timestamp
generate_seriesreturns a LIST in thatposition instead of a set, and the read fails with:
It is the placement that matters, not the arguments; the failure
reproduces with all-literal arguments. Moving the call into
FROMworks on both backends, so the change is applied unconditionally
rather than behind a flag.
Changes
Both sites are the
all_bucketsCTE, inBuildMetricsQueryandBuildDerivedMetricsQuery.Before:
After:
server/src/internal/metrics/query.go: bothall_bucketsCTEs.server/src/internal/metrics/query_test.go: the existing derivedquery assertion checked only the
generate_series(...)substring,which both forms contain; it now asserts the full
FROM generate_series(...) AS g(bucket_time)form. An equivalentassertion is added to
TestBuildMetricsQueryso the placement ispinned at both sites.
Testing
cd server && make testpasses;gofmt,go vet, andgolangci-lint run ./internal/metrics/...are all clean.BuildMetricsQueryandBuildDerivedMetricsQueryare both at 100%statement coverage in
go tool cover -func.Summary by CodeRabbit