Place the Query Store slicer overlay where the work ran (#1921, option 1)

[erikdarlingdata]

Fixes #1921. Erik's call: option 1 — the overlay becomes work-placed-when-it-ran.

The disagreement

Selecting a Query Store row overlays that item's curve on the slicer. #1841 moved the bars to the hour an
interval ran and left the overlay on the hour it was collected, so a point sat up to one Query Store
interval — 60 minutes by default, 1440 at the maximum — to the RIGHT of the bar describing the very same work.

The file's own comment stated the invariant that broke:

leaving the overlay raw would make the overlay disagree with the bars it annotates

Its dedup half kept holding. Its placement half silently stopped.

The fix

Both apps place the overlay on COALESCE(interval_start_time_utc, collection_time) — the same expression the
bars use — and pre-tier-2 rows keep collection-time placement, the same two-generation handling every other
post-#1841 read uses.

The window moved with the x-value, which is the part that would otherwise still disagree. Filtering on
collection time while plotting on the interval start disagrees at both edges — an interval that started
before the window but closed inside it draws a point to the left of the range, and the window's own last
interval, whose closing fetch lands after the range ends, vanishes — and worse, it disagrees with the bars
about whether an interval is in the window at all. Same collection_time pruning bounds the slicer already
ships (#1892/#1923); the issue's own "no ceiling" scope note is stale, written before that round added one.

ORDER BY moved too — ordering on collection time while plotting on the interval start hands the chart a
non-monotonic series, which a line control draws as a zig-zag rather than a curve.

Two things the brief didn't anticipate

1. The nominated watched-red pin does not move — and shouldn't.
QueryStoreAggregates_CountARecollectedIntervalOnce_... seeds its rows without an interval identity
(intervalId/intervalStartUtc default to NULL), so COALESCE correctly falls back and the point stays at
bucketStart+15m. It passed before and after the change.

Rather than force it, its assertion is unchanged and now pins the legacy fallback, and the behavioral
proof went into QueryStoreIntervalIdentity_PlacesWorkWhenItRan_..., whose tier-2 rows genuinely disagree —
the interval starts in h0 and every collection lands in h1. Watched red there: reverting the x-value fails
it with Expected 06:00, Actual 07:10, the 70-minute offset, while the legacy pin keeps passing. Both
generations are now covered by the test whose data can actually demonstrate each.

2. Lite has no dedicated overlay read.
It fed the overlay from the history grid's read — a deliberately raw per-collection projection (#1845)
windowed on collection time. That is right for a list of collection events and wrong for a series drawn over
the bars, and moving the shared read would have silently changed the history grid's window too, a surface
outside this issue.

So Lite gained a dedicated overlay read, matching the structure Darling has had since #683. It picks up the
dedup the overlay never had: an interval collected repeatedly used to draw as a rising staircase of
restatements and now draws as one point at its final values — which is what the bar beside it always showed.
That is the same invariant ("the overlay agrees with the bars"), not extra scope.

Testing

Watched red on the behavioral pin, as above. Both source guards caught the new read and were updated
deliberately — 5 → 6 dedup sites, 7 → 8 rank filters, tie-break total 7 → 8. The #1912 tie-break guard fired
from the direction of a read being added rather than edited, which is exactly what it was written for.

New live-DuckDB pin Overlay_PlacesPointsWhereTheWorkRan_AgreeingWithTheBarsItIsDrawnOver asserts the
invariant as an assertion rather than a comment: same x and same value as the bar computed from the same rows.

Sibling timelines are untouched and correctly so — query_stats/procedure_stats are per-collection deltas
with no interval concept, so they stay on collection_time.

Suites: Darling 4033 passed / 0 failed against live PostgreSQL 18.4 + TimescaleDB 2.28.1 (10 skipped =
gated); Lite 1937 passed / 0 failed. Full-solution -t:Rebuild: 0 warnings, 0 errors.
Installer.Tests deliberately not run.

Accepted costs, recorded so they aren't re-reported as bugs

The overlay can no longer show WHEN a value was observed — a real thing to want, and what option 2 would have
kept — and several intervals from one collection cycle now spread across the x-axis instead of stacking.
Spreading is the truthful picture: those intervals really did run at different times.

Deferred

None.

🤖 Generated with Claude Code

[claude]

Review: Place the Query Store slicer overlay where the work ran (#1921)

Overview

Fixes the overlay/bar disagreement introduced by #1841: the slicer bars moved to COALESCE(interval_start_time_utc, collection_time) but the per-item overlay stayed on collection_time, so a point could sit up to one Query Store interval to the right of the bar for the same work. Both apps now place, window, and order the overlay on the same expression the bars use, with legacy (pre-tier-2) rows falling back to collection_time via COALESCE. Lite additionally gains a dedicated GetQueryStoreItemTimelineAsync overlay read (previously it reused the raw, undeduped history-grid read), bringing it in line with the structure Darling has had since #683.

Correctness

Traced the math through both the DuckDB-backed live test (Overlay_PlacesPointsWhereTheWorkRan_AgreeingWithTheBarsItIsDrawnOver) and the live-Postgres test (QueryStoreIntervalIdentity_PlacesWorkWhenItRan_...) — the expected CPU/elapsed values match the seeded avg_* x execution_count products, and the point placement/dedup/ordering logic is sound:

• COALESCE correctly falls back to collection_time for pre-tier-2 rows (no interval_start_time_utc), verified by the legacy pin staying unchanged.
• The window filter now matches the placement expression (COALESCE(...) BETWEEN $5 AND $6), avoiding the two-edge disagreement described in the PR body.
• The wider collection_time bounds ($5 - interval '1 day' / $6 + interval '30 days') are pruning-only (chunk exclusion / parquet row-group skip), matching the existing Query Store time-slice: window filters on collection_time but buckets on interval_start_time_utc, so a bucket can fall outside the requested range #1892/Fix the three #1895 residue items: per-monitor dialog clamp, Query Store window edges, cpu.mode vocabulary #1923 shape verbatim on both apps.
• ORDER BY moved from collection_time to point_time to keep the series monotonic on its own x-axis, matching the CTE's actual output column.

Parity

Verified both apps received matching treatment:

• Darling's ViewerDataService.ItemTimeline.cs and Lite's new GetQueryStoreItemTimelineAsync implement the same placement/window/order/dedup logic.
• Lite's source-count guards were updated consistently: EveryQueryStoreAggregateInTheFile_CarriesADedupCte (5 to 6 partitions, 7 to 8 rank filters) and QueryStoreSliceTieBreakSourceTests (7 to 8 total) - spot-checked with grep -c against the actual file and both match the asserted counts.
• Darling's analogous QueryStoreSliceTieBreakSourceTests guard (12 total) doesn't need updating, since it targets the window function's internal ORDER BY collection_time DESC, execution_count DESC (unchanged) rather than the outer result ORDER BY (which did change) - confirmed this is not a gap.

Minor findings (posted inline)

1. Three files pick up an unintended UTF-8 BOM at the very first line (confirmed absent on the merge-base revision) - likely an editor/tool artifact rather than a deliberate change.
2. A naming nit: Darling's shared ItemTimelinePoint.CollectionTime field now holds point_time (possibly the interval start, not the collection time) for the Query Store overlay, while Lite's brand-new equivalent type calls the same concept PointTime. Not a bug - the shared record is reused by the (still collection-time-based) query_stats/procedure_stats overlays - but worth a look for future clarity.

Overall this is a very thorough, well-tested fix with good live-DB coverage on both apps; no correctness or parity issues found beyond the nits above.

[claude]

This file (and Lite.Tests/QueryStoreDedupReadTests.cs, Lite.Tests/QueryStoreSliceTieBreakSourceTests.cs) picked up a leading UTF-8 BOM in this PR — confirmed absent on the merge-base revision (git show 4405108:<path> | head -c 3 is empty; this PR's head has EF BB BF). It's harmless for the C# compiler, but it's inconsistent with the rest of the codebase and is likely an accidental editor/tool artifact worth stripping before merge.

[claude]

Minor naming nit: the query now aliases this as point_time, but it's read back into the shared ItemTimelinePoint.CollectionTime field (declared above at line 34-35). For Query Store rows that field can now hold the interval start rather than the actual collection time, while Lite's brand-new QueryStoreItemTimelinePoint names the equivalent concept PointTime. Understandable given ItemTimelinePoint is shared with the still-collection-time-based query_stats/procedure_stats overlays, but worth a look for clarity (e.g. a distinct record, or renaming the field) since "the point is placed where the work ran, not observed" is the whole thesis of this PR.

[claude]

Same accidental leading UTF-8 BOM as Lite/Services/LocalDataService.QueryStore.cs and Lite.Tests/QueryStoreSliceTieBreakSourceTests.cs — absent on the merge-base revision, worth stripping.

[claude]

Same accidental leading UTF-8 BOM as Lite/Services/LocalDataService.QueryStore.cs and Lite.Tests/QueryStoreDedupReadTests.cs — absent on the merge-base revision, worth stripping.

[erikdarlingdata]

PR UP — #1938, all checks green, 46b3d9e6, MERGEABLE / CLEAN (checked right after opening too).

Two things the brief didn't anticipate, both verified before acting rather than worked around:

1. The nominated watched-red pin does not move — and shouldn't. QueryStoreAggregates_CountARecollectedIntervalOnce_... seeds its rows without an interval identity, so COALESCE correctly falls back and the point stays at bucketStart+15m. It passed before and after. Rather than force it, its assertion is unchanged and now pins the legacy fallback; the behavioral proof went into QueryStoreIntervalIdentity_PlacesWorkWhenItRan_..., whose tier-2 rows genuinely disagree (interval starts h0, collections land h1). Watched red there: reverting the x-value fails with Expected 06:00, Actual 07:10 — the 70-minute offset — while the legacy pin keeps passing. Both generations now covered by the test whose data can demonstrate each.

2. Lite has no dedicated overlay read. It fed the overlay from the history grid's read — a deliberately raw per-collection projection (#1845) windowed on collection time, right for a list of collection events and wrong for a series drawn over the bars. Moving that shared read would have silently changed the history grid too, a surface outside this issue. Lite gained a dedicated read instead, matching Darling's structure since #683 — and it picks up the dedup the overlay never had, so a repeatedly-collected interval now draws as one point at its final values instead of a rising staircase. Same invariant, not extra scope.

Also followed your correction over the issue text: the + interval '30 days' pruning ceiling from #1923 is in, and ORDER BY moved with the x-value (a non-monotonic series draws as a zig-zag).

Both source guards caught the new read and were updated deliberately (5→6 dedup sites, 7→8 rank filters, tie-break total 7→8) — the #1912 guard fired from the direction of a read being added rather than edited, which is what it was written for.

Darling 4033/0 against live PG 18.4 + TSDB 2.28.1, Lite 1937/0, -t:Rebuild 0 warnings. PostgreSQL on 5619 is down, verified.

[claude]

Review

Overview: Fixes #1921 by placing the Query Store slicer overlay on COALESCE(interval_start_time_utc, collection_time) — the same expression the bars already use since #1841 — instead of raw collection_time. The window filter and ORDER BY move with it, and Lite gains a dedicated GetQueryStoreItemTimelineAsync overlay read (previously it borrowed the raw, undeduped history-grid read), bringing it in line with the structure Darling has had since #683.

Correctness

• Verified the two-generation handling: COALESCE correctly falls back to collection_time for pre-Query Store aggregates double-count re-collected cumulative intervals - tier 2: interval identity + CAGG rebuild (tier 1 read-side dedup shipped in #1845) #1841-tier-2 rows with no interval identity, and the new behavioral test (QueryStoreIntervalIdentity_...) exercises the case where the two clocks actually disagree (interval starts in h0, both collections land in h1), which the older dedup-fallback test structurally can't do.
• The new floor/ceiling chunk-exclusion bounds (collection_time >= $5 - 1 day / <= $6 + 30 days) can't exclude any row the primary predicate keeps — traced through the logic and it holds (interval_start_time_utc ≤ collection_time always, so the floor is implied; the ceiling is the same generous heuristic already shipped for the slicer in Query Store time-slice: window filters on collection_time but buckets on interval_start_time_utc, so a bucket can fall outside the requested range #1892/Fix the three #1895 residue items: per-monitor dialog clamp, Query Store window edges, cpu.mode vocabulary #1923).
• ORDER BY point_time is required now, not cosmetic — collection_time isn't even a column of the deduped CTE output anymore, so the old order-by would fail to compile, and ordering by it while plotting on interval-start would produce a non-monotonic series anyway.

Lite/Darling parity

• Both apps place, window, and order on the same COALESCE expression; both dedup with the same PARTITION BY ... ORDER BY collection_time DESC, execution_count DESC shape and the same Query Store: the in-memory and flushed slices of one runtime_stats_id collide in the read-side dedup, understating execution counts non-deterministically #1907 tie-break rationale.
• Confirmed the guard-count updates are actually correct, not just asserted: PARTITION BY database_name, query_id, plan_id, runtime_stats_interval_id, first_execution_time, execution_type_desc, replica_role occurs 6 times and rn = 1 occurs 8 times in LocalDataService.QueryStore.cs, matching the updated EveryQueryStoreAggregateInTheFile_CarriesADedupCte / tie-break-source assertions.
• The ItemTimelinePoint.CollectionTime → PointTime rename is applied consistently across Darling's shared record, its three SQL sources, ReadItemTimelineAsync, and the slicer-overlay consumer (ViewerServerTab.SlicerOverlay.cs) — no stale references found. query_stats/procedure_stats correctly keep plain collection_time (no interval concept), matching the PR's own claim.
• Lite's history-grid double-click drilldown (GetQueryStoreHistoryAsync, Dedup Query Store intervals before aggregating (#1841 tier 1) #1845) is untouched, as claimed — only the slicer-overlay caller in ServerTab.Grids.cs was rewired to the new dedicated read.

Style/observations (non-blocking)

• The SQL comment blocks in the Darling and Lite versions of this read are near-duplicates of each other (by necessity, since each app owns its own SQL string) — consistent with how the rest of the file already handles this, not a new pattern.
• Nothing else stood out — parameter binding, null handling (IsDBNull guards on all nullable numeric columns), and the accepted-cost tradeoffs (loses "when observed", intervals from one collection cycle now spread across the x-axis) are explicitly disclosed in comments/changelog rather than silently introduced.

No correctness, parity, or security issues found. The test coverage (behavioral pin using data that can actually distinguish the two placements, plus updated source-shape guards) is a good match for a bug that's easy to silently reintroduce.