Repository navigation
issue-3653 A6 lane LA: stitched rollup reads, every hourly reader routes legacy→successor at the successor floor - #4182
Conversation
Lane A6-LA-2 status: 3 of 10 hourly readers routed; 7 remain unroutedRouted through
NOT done — still unrouted, left on
Also not done:
Why stopped here: hit the 150k/20-minute context-wall checkpoints from the dispatch rules before the FinOps builder edits + build verification were even committed; per the rules I stopped investigating the remaining 7 readers (several need real design decisions — CHANGELOG entry
|
LA-3b (route TrendReader / DailySummary / HealthReader hourly readers through
|
…yFromClause/ProbeHourlyView fields Adds the two FROM-clause/probe-name fields decisions 1/2/4 require to DurationTrendRoute and wires ResolveDurationTrendRoute to compute HourlyFromClause via RollupCoverage.StitchedRelationSql. The splice into BuildBucketedHourlyTrendSql/QueryHistoryHourlySqlFor and the successor-aware probe wiring are NOT done yet -- ran out of budget mid-step. See PR #4182 body section LA-3b1 for the handoff.
LA-3b1Status: partial, budget exhausted mid-step. Pushed What landed
What's NOT done (next lane must pick up here)
Verification runNone completed. Why this landed half-builtInvestigation (reading CHANGELOG entry(none yet — no user-visible behavior change has landed; the next lane's PR should carry the entry once the splice is byte-identical-pinned and proven live.) |
LA-3b2Status: code landed and builds clean; NOT tested (context wall hit before a test run). No test file added. What changed (3 files, per brief)
Verified
NOT verified (ran out of budget before this)
Recommendation for a follow-upA next lane (or this lane's continuation) should:
No files outside |
|
Coordinator note check (compile-after-58c917e2b): verified — |
LA-4bStatus: steps 1–3 done and green. Step 4 (flip the scan test) and the full suite (step 5) NOT done — LA-4a had not pushed to What changed
NOT done (budget ran out)
Verified
RED findingsNone — both stitched callers ( For the next lanePull, confirm LA-4a's CHANGELOG entry
|
LA-4aSteps 1–3 done, pushed to Step 1 — Step 2 — Step 3 — new Step 4 — NOT DONE. Ran out of context budget before starting the live PG proof (create the two overlapping-bucket rollups, run a real read through the reader, check per-bucket totals split exactly at F). Whoever picks this up: rig is Known collateral, not fixed (out of my file scope): Also ran |
LA-5 (closeout)Five items, all done on
Not verified: the full suite (see above), and whether CHANGELOG entry
|
LA-6Ruling applied (LA-5's 3 offenders + doc-hygiene):
Targeted, green: Full suite, once, Not verified: live-PG proof on Branch: pushed to |
Adds the stitch builder from design-3653-A6-v2.md \u00a72: a FROM-clause item that reads a frozen legacy rollup and its interval-honest successor as one relation, split at the successor's first bucket, on both the hourly and daily tiers. - Three shapes: legacy-only (no successor / absent / empty legacy), successor-only (no legacy, empty legacy, or successor already reaches the window start), or a UNION ALL split at a literal boundary F. - Daily boundary F_d = max(FloorOf(successorDaily), ceilDay(FloorOf(successorHourly))), keeping the successor hourly's partial first day on the legacy side. - Per-pair explicit column lists (no sample_interval_seconds_sum; no reader selects it). - LA carries a private stub of the daily registry (LegacyDaily, SuccessorDaily, SuccessorHourly) marked for LB to replace with SupersededDailyRollups. Until LB lands, the daily stitch is inert (legacy-only) on every store, by construction. Not yet done in this PR (see PR body): routing the hourly- and daily-tier readers listed in \u00a71 through the builder, the live PG proof, and the RollupCoverageRoutingTests Resolve-call scan. Ran out of budget on step 1; the routing steps are the PR's open work. Part of #3653 (A6, lane LA)
…ugh StitchedRelationSql
…edRelationSql ComposeRoute gains CaggFromClause (decision 2): CaggRelation stays the by-name answer for probes/logs; CaggFromClause is the FROM-clause item the compiler splices in, computed by RollupCoverage.StitchedRelationSql so a stitch applies when a successor's floor sits inside the window. With no successor this is byte-identical to collect.<view> AS f. ComposeCompiler's BuildFactRelation/AppendFactBody use CaggFromClause when present and skip the alias it already carries.
…gh StitchedRelationSql ReadRoutedDurationTrendAsync now builds the hourly FROM-clause item via RollupCoverage.StitchedRelationSql instead of HourlyRelationFor's bare name, so a stitch applies when a successor's floor is inside the window. With no successor the FROM clause is byte-identical to collect.<legacy> AS f, and the code takes the hourlySql fast path exactly as before.
…hrough StitchedRelationSql GetQueryTrend now resolves the hourly relation via RollupCoverage.StitchedRelationSql rather than the by-name HourlyRelationFor, matching QueryHistoryHourlySqlFor's FROM-clause consumer. With no successor the text is byte-identical to today's legacy relation name.
… until LA-3b lands) NoReaderOutsideTheBuilder_CallsHourlyRelationForDirectly walks Darling/ for HourlyRelationFor( call sites outside TimescaleSupport.cs (the builder). Marked [Fact(Explicit = true)] because LA-3b's readers (DarlingTrendReader, DailySummary, DarlingHealthReader) still call it by name; LA-3b flips it to a plain [Fact] once all ten readers route through StitchedRelationSql.
…yFromClause/ProbeHourlyView fields Adds the two FROM-clause/probe-name fields decisions 1/2/4 require to DurationTrendRoute and wires ResolveDurationTrendRoute to compute HourlyFromClause via RollupCoverage.StitchedRelationSql. The splice into BuildBucketedHourlyTrendSql/QueryHistoryHourlySqlFor and the successor-aware probe wiring are NOT done yet -- ran out of budget mid-step. See PR #4182 body section LA-3b1 for the handoff.
…carried probe - RollupCoverage.StitchFloor: exposes the same boundary StitchedRelationSql splits at, or null when that call would answer legacy-only/successor-only. - DailySummarySql.QueriesCteForStitchedCagg: runs the not-carried probe (and the rollup half) twice, once per relation name split at F, UNION ALL'd, per decision 3. - New DailySummarySql.RangeSqlFor(tier, coverage, windowStartUtc) overload: stitches when StitchFloor is non-null, else delegates to the existing RangeSqlFor(tier, hourlyRelation) unchanged. - ViewerDataService.DailySummary.cs and DarlingHealthReader.cs now call the new overload instead of resolving HourlyRelationFor + RangeSqlFor by name.
… RangeSqlFor overload
…ead by the daily summary and one Compose query
…rendReader's builders and probe
…he pin, add live query-history proof
… add HourlyRelationNameFor for by-name uses, fix doc-hygiene - DailySummarySql.RangeSqlFor(tier, coverage, windowStartUtc): the legacy-only branch now reads its relation name back off StitchedRelationSql's own splice instead of calling HourlyRelationFor directly, so every SQL splice in the file goes through the builder (decision 1). Byte-identical to before: the branch only runs when StitchFloor answered null, which is exactly where the two calls agree. - TimescaleSupport.RollupCoverage: added HourlyRelationNameFor(legacyHourly, windowStartUtc), a thin by-name accessor for probes/logs/registry lookups, documented as never to be spliced into SQL. - ComposeSourceRouter.cs and DarlingTrendReader.cs: switched their by-name uses (CaggRelation, DurationTrendRoute.HourlyView) onto HourlyRelationNameFor, leaving their SQL splices on StitchedRelationSql as before. - DocCommentHygieneTests: fixed RawTableFor's doc comment, which had two </summary> for one <summary> (a stray early close before the <para> block) — a pre-existing doc-run defect the scan test caught. Targeted classes green: RollupCoverageRoutingTests (40), DailySummaryNotCarriedTests (6), ComposeSourceRouterTests (26), IntervalHonestHourlyRollupTests (8), DocCommentHygieneTests (77), DailySummaryStitchedRangeTests (5), StitchedRelationSqlTests (11), DarlingTrendReaderStitchTests (9), DarlingQueryTrendTieringTests (12), QueryStoreTrendRoutingTests (12). Full suite once, DARLING_TEST_PG unset: 13729 total, 214 failed (matches the documented Mac base), 760 skipped. ViewerTrendRoutingPortTests has 2 pre-existing PresentationFramework FileNotFoundExceptions on this Mac, unrelated to this change.
cf23276 to
e2e007f
Compare
…hygiene comment, rewrite stale pre-A6 expectation
- (A) hygiene: add the own-store NOT [Collection("live-postgres")] doc comment to
DailySummaryAndComposeStitchedLiveTests, in PgDeadlockRemaskTests' shape.
- (B) the real bug: DailySummarySql.QueriesCteForStitchedCagg split every {boundary} at the
successor's floor F's own HOUR. A mid-day F let the floor day supply a row from each side of
the UNION ALL, doubling that one calendar day with split distinct counts. Fixed by computing a
DAY-aligned boundaryDay (F itself if already a day start, else F's date plus one day) and using
it everywhere {boundary} appears. Proved red first (a temporary boundaryDay = successorFloor
reproduced 3 rows where 2 were correct on the new live test), then green with the fix.
- (C) DailySummaryNotCarriedLiveTests' recent-server leg asserted the pre-A6 supply rule (the
reader carries the legacy's whole answer). Since LA-3b2 the reader stitches at F, day-aligned;
rewrote the leg to the stitched answer, derived by running it live: days [R0,R2,R3,R4], counts
[3,null,7,2], days_missing=[R2].
- New tests: a pure pin in DailySummaryStitchedRangeTests that the SQL literal is the day-aligned
boundary, not F's own hour, and a new live test
(DailySummary_MidDayFloor_PrintsExactlyOneCalendarRow_NotTwo) that seeds a mid-day floor and
pins exactly one calendar row for the floor day.
- Updated DailySummaryAndComposeStitchedLiveTests' existing stitched-pair leg: with the
day-aligned boundary, D2 (where F itself falls) now reads whole from the legacy, so its count
is 2 (was 1), and the comment explaining the old 'F lands exactly on a day start' framing is
corrected.
Part of #3653 (A6, lane LA-7).
LA-7Fixed PR #4182's two CI failures plus the pre-existing leg the fix invalidates. (A) hygiene, mechanical. Added the (B) the real bug. Fix: Proved RED first: temporarily set (C) stale pre-A6 expectation. New/updated tests:
Ran (live, Did NOT run: the full Darling suite (context budget did not permit it this lane; left for the coordinator/pr-tender to run once before arming). CHANGELOG entry
|
Part of #3653 (A6, lane LA)
What this adds
RollupCoverage.StitchedRelationSql(legacy, alias, windowStartUtc, tier)(design-3653-A6-v2.md §2), onTimescaleSupport.csnext toHourlyRelationFor. It returns one of three FROM-clause items:collect.<legacy> AS alias— no successor (an unsuperseded pair, or the successor absent from availability, or the legacy is absent/empty and the successor is also absent/empty).collect.<successor> AS alias— no legacy, the legacy is empty, or the successor's floor already reaches at or beforewindowStartUtc.(SELECT <cols> FROM collect.<legacy> WHERE bucket < F UNION ALL SELECT <cols> FROM collect.<successor> WHERE bucket >= F) AS alias— otherwise, F a UTCTIMESTAMPliteral.For the daily tier,
F_d = max(FloorOf(successorDaily), ceilDay(FloorOf(successorHourly))), keeping the successor hourly's partial first day on the legacy side of the split, as the design requires.Each pair (hourly and daily, all three families) has its own explicit column list — the legacy's own columns, with no
sample_interval_seconds_sum(no reader selects it). LA does not own the daily registry: I added aprivate staticstub list of (LegacyDaily, SuccessorDaily, SuccessorHourly) marked// LB replaces with SupersededDailyRollups. Until LB lands,RollupAvailabilityhas no field for the successor dailies, so on every store today the daily stitch is legacy-only by construction — inert, as the brief specifies.Pure tests in
Darling.Tests/StitchedRelationSqlTests.cs(11 cases, all green): legacy-only byte-identical text, successor absent from availability, legacy empty, successor empty, successor fully covering the window, a partial stitch with its column list and nosample_interval_seconds_sum, a stale-but-safe F (no gap/overlap), every hourly pair inSupersededHourlyRollups, the daily-inert case, and the daily ceiling-of-day boundary (mid-day successor-hourly floor → boundary is the start of the next day, not the partial bucket). Also reranIntervalHonestHourlyRollupTestsandRollupCoverageRoutingTests(47 tests) — unaffected, all green.What is NOT done in this PR — I ran out of context budget on step 1
The brief's steps 2–5 (routing every hourly- and daily-tier reader listed in design-3653-A6-v2.md §1 through the builder —
ComposeSourceRouter.cs,DarlingTrendReader.cs,DailySummarySql.cs's two-sided probe,FinOps.Workload.cs,DurationTrendRouting.cs,QueryTrends.cs,ViewerDataService.DailySummary.cs,DarlingHealthReader.cs,DarlingMcpTrendTools.cs), the live-PG proof (seed both sides, check the per-bucket totals split exactly at F with no gap/overlap), and theRollupCoverageRoutingTestsResolve-call scan are not implemented. I read every named call site (§1) and located the relevant fields (ComposeRoute.CaggRelation,DurationTrendRoute.HourlyView,DailySummarySql.QueriesCteForCagg's target lookup) before running out of budget on the builder itself, which is why this PR does not build on top of a broken foundation — but it means the design's read-side promise ("stitch both tiers so the freeze can ship now") is not yet delivered end to end. The successor's builder is proven correct in isolation only.What I did NOT verify
timescale/timescaledbcontainer was run).RollupCoverageRoutingTests.EveryProductionRoutingCaller_PassesCoverage) was not touched or extended to cover the new builder's call sites, because there are none yet.DARLING_TEST_PGunset) was not run — only the new test class plus the two named-in-brief classes.CHANGELOG entry
Added
RollupCoverage.StitchedRelationSql, a SQL builder that stitches a frozen legacy rollup to its interval-honest successor at the successor's first bucket (both the hourly and daily tiers). Not yet wired into any reader.LA-3a
Routed 3 more hourly-tier readers through
RollupCoverage.StitchedRelationSql, per decisions 1/2/5:ComposeSourceRouter.cs~:325-343,ComposeRoute,ComposeCompiler's FROM site).ComposeRoutegainedCaggFromClause(decision 2), computed alongside the existingCaggRelation(which stays the by-name answer for probes/logs).ResolveFamilysetsCaggFromClauseviacoverage.StitchedRelationSql(hourlyView, "f", windowStartUtc, StitchTier.Hourly).ComposeCompiler.BuildFactRelationreturnsroute.CaggFromClausewhen present (a complete, aliased relation already), andAppendFactBodyskips its own" AS f"suffix in that case to avoid double-aliasing. With no successor the FROM clause iscollect.<view> AS f, byte-identical to what the compiler produced before.ViewerDataService.QueryTrends.cs,ReadRoutedDurationTrendAsync). Replacedcoverage.HourlyRelationFor(...)withcoverage.StitchedRelationSql(hourlyView, "f", startUtc, StitchTier.Hourly). When the stitch answers exactlycollect.<legacy> AS f, the code still takes the pre-builthourlySqlfast path (byte-equal to the MCP reader's pinned constant); otherwise it builds fresh SQL viaDurationTrendRouting.BuildHourlyTrendSql(hourlyFromClause, withDatabaseFilter: true)— that builder just interpolates its argument intoFROM {arg}, so a full FROM-clause item (bare-name or stitched UNION ALL) slots in without any other change.Mcp/DarlingMcpTrendTools.cs~:387,GetQueryTrend). Replaced theHourlyRelationForcall feedingGetQueryHistoryAsync'shourlyRelationparameter withcoverage.StitchedRelationSql(TimescaleSupport.QueryStatsHourlyView, "f", now.AddHours(-hours_back), StitchTier.Hourly).QueryHistoryHourlySqlForalso just doesFROM {hourlyRelation}, so this is the same drop-in.Scan test (decision 5). Added
RollupCoverageRoutingTests.NoReaderOutsideTheBuilder_CallsHourlyRelationForDirectly: walksDarling/(sameFindRepoRoot/IsExcludedFromScan/StripCommentshelpers the existingEveryProductionRoutingCaller_PassesCoverageguard uses) forHourlyRelationFor(call sites outsideTimescaleSupport.cs(the builder bothHourlyRelationForandStitchedRelationSqllive in). Marked[Fact(Explicit = true)]per the brief: LA-3b's readers (DarlingTrendReader.cs's route + query-history read at ~:912/:1323,DailySummary.cs/DailySummarySql.cs,DarlingHealthReader.cs) still callHourlyRelationForby name as of this lane's last push, so the guard would start red. Ran it with-explicit only: it correctly FAILS right now (offenders in those three files), confirming the scan itself works. LA-3b should flip it to a plain[Fact]once all ten readers route through the stitch.Tests run per step (this Mac,
DARLING_TEST_PGunset,Microsoft.WindowsDesktop.Appstripped fromDarling.Tests.runtimeconfig.jsonfor each run then restored):ComposeSourceRouterTests: 26/26 pass.DarlingComposeTests: 280/280 pass.ViewerTrendRoutingPortTests: 12/14 pass, 2 pre-existing failures (DescribeQueryStoreTrendCoverage_NamesTheRoute_AndTheHeadOnlyWhenUnserved,DescribeTrendCoverage_NamesTheTier_AndTheHeadOnlyWhenTruncated) — confirmed red onorigin/fix/3653-a6-stitched-readsBEFORE my QueryTrends.cs edit too (stashed my change, rebuilt, same 2 failures), so not introduced by this lane.IntervalHonestHourlyRollupTests: 8/8 pass (both before and after step 3).DarlingPerformanceTrendsReadTests: 1 skipped (needsDARLING_TEST_PG), 0 failed.RollupCoverageRoutingTests: 40/40 non-explicit pass; the new Explicit test correctly fails under-explicit only(LA-3b not landed yet), correctly not-run under the default-explicit off.What is NOT done in this LA-3a slice
The live Compose proof (seeded stitched pair, one live Compose read, per-bucket totals split at F with no gap/overlap) was NOT completed — I hit the context-wall warning before reaching it and prioritized landing the routing + scan test, which are the harder-to-recreate parts, with everything green and pushed.
IntervalHonestHourlyRollupLiveTests(same file) has the exact rig to copy:ScratchPostgres.CreateAsync+PgMigrations.MigrateAsync+TimescaleSupport.TryEnableAsync/ConvertToHypertablesAsync+ seedcollect.query_statsrows spanning two hours +EnsureContinuousAggregatesAsync+RollupBackfill.RunSliceAsyncon both the legacy and successor hourly views + a realComposeCompiler.Compile/execute over a plan whose window straddles the successor floor. A follow-up (or LA-3b, if it has budget left after flipping the scan test) should add this as a[Fact]inDarlingComposeTests.csor a newComposeStitchedRelationLiveTests.cs, asserting the legacy rows count below F and successor rows at/above F with none double-counted.What I did NOT verify
DarlingComposeTestsline ~905 (Compile_OldWindow_QueryStore_...) tests I did not touch would need re-pinning if a stitch is ever seeded through them — they useRollupCoverage.Unknown/hand-builtRollupCoverage, notStitchedRelationSql, and stayed green, so no.