Skip to content

fix: keep AAS extremes honest across rollup grains (#210) - #220

Draft
NikolayS wants to merge 2 commits into
mainfrom
release/fix-grain-honesty-210
Draft

fix: keep AAS extremes honest across rollup grains (#210)#220
NikolayS wants to merge 2 commits into
mainfrom
release/fix-grain-honesty-210

Conversation

@NikolayS

@NikolayS NikolayS commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Fixes #210 — the largest blocker from the pre-tag audit (#160). ash.top() and ash.compare()
report hour-grain numbers in columns named peak_aas / p99_aas, so compare() fabricates
regressions on byte-identical load.

Draft on purpose. This is the riskiest change in the release: it alters a user-visible contract
on eight readers and changes ash.compare()'s return signature. Needs REV and your approval.

The rule implemented

NULL peak_aas / p99_aas whenever the read grain is coarser than the requested bucket.

Not an arbitrary pick — ash.timeline() already follows exactly this rule for filtered rollup_1h
reads, with the shipped comment "a percentile over hour averages would masquerade as a minute
percentile"
. aas and top were violating an invariant this same file writes down two functions
away. This makes the file self-consistent.

Sub Fix
B1a top() no longer emits hour averages under peak_aas/p99_aas. avg_aas, backend_seconds, pct stay valid.
B1b aas() follows the same rule when a filter forces hour grain.
B1c compare() gains source_1 / source_2, and suppresses all four peak/p99 values when the two windows resolve to different grains. Average deltas remain valid.
database dim Fixed properly rather than NULLedrollup_1h.minute_counts is stored per (ts, datid), so the minute detail already existed and simply wasn't read. Now expanded through rollup_1h_minutes, keeping true minute-grain peak/p99. AAS_API.md §2.4's justification ("the per-dimension arrays are stored per hour") was false for this dimension and is corrected.
B1d Legacy hours (minute_counts IS NULL, ~98% of a mature 1.x→2.0 archive) now report source = 'rollup_1h_flat' instead of masquerading as measurement.

On B1d's marker choice: rollup_1h_flat was chosen over data_points = 0 because those hours
still hold usable average and backend-second evidence — zero data points would incorrectly signal
no coverage. That reasoning is sound and I'd keep it, but it's a contract decision worth your
confirmation.

Verification

Tests were committed before the implementation (98e4c2c test: then e135d22 fix:) — the TDD
ordering is visible in the history, not just claimed.

Deterministic fixture: 59 minutes at AAS 1 plus one minute at AAS 10 → 4,140 backend-seconds,
avg 1.15, peak 10.00, p99 4.69. Every expected number is hand-computed.

RED against unpatched SQL, showing the actual wrong values:

ERROR: B1a top key=CPU* source=rollup_1h avg=1.15 peak=1.15 p99=1.15 seconds=4140.00
ERROR: B1b filtered source=rollup_1h avg=0.17 peak=0.17 p99=0.17 seconds=600.00
ERROR: record "v_dim" has no field "source_1"
ERROR: database spike source=rollup_1h avg=1.15 peak=1.15 p99=1.15 pct=36.51
ERROR: B1d aas source=rollup_1h expected=60 with_data=60 avg=0.17 peak=0.17 p99=0.17

GREEN: all eight blocks pass, including cross-reader agreementaas, timeline, top,
summary and report now assert to the same 1.15 / 10.00 / 4.69 / 4140.00 for one fixed window.
That agreement is the whole point of the fix, so it is asserted directly.

Also verified:

  • Docker matrix green on PG 14, 15, 16, 17, 18 and 19beta1
  • fresh install, full 1.0→2.0 chain, double re-apply
  • fresh-vs-upgrade semantic schema snapshots: 288 lines each, zero diff
  • compare()'s new signature identical on both paths
  • zero-argument sweep of every reader (the regression shape fix: reject partial hourly drills #131 currently trips on)

I separately confirmed the riskiest mechanic by hand: a real v2.0-beta1 install upgraded with
this branch's installer ends up with source_1 text / source_2 text present and compare()
callable. CREATE OR REPLACE cannot change a return type, so this depends on the installer's
dynamic drop block — it works, but that is the thing to re-check if anything here is rebased.

⚠️ Collides with #131 and #150 — neither is mechanically safe to merge

Both already collide with each other; this is a third party. Do not resolve any of it with
-X ours / -X theirs.

vs #131 — overlaps aas 3885–3896, timeline 4042–4063, top 4470–4489, catalog
5752–5764 (all four function comments), and workflow 3837–3992. Its chart hunk 5519–5560 is
separable. A human must rebuild its partial-hour policy around the new requested/read/effective
grain logic, preserve the database minute-detail exception, and keep now-relative and default
calls working. Its unconditional alignment guard must not be carried over — that is what breaks
zero-arg ash.summary().

vs #150 — overlaps _grain_counts ~3578, aas 3800–3867, timeline 3950–4032,
_grain_by 4179–4359, top 4376–4507, catalog 5752–5764, blueprint 36–45, 138,
174–204, 381–400, workflow 3712–3871. Order matters: in _grain_by, keep this branch's early
database-minute expansion then apply #150's query-attribution branch; in top, apply #150's
exact-query decision first, then this branch's source / legacy-flat / extreme-validity logic.
compare()'s new signature belongs here, but its delegated reads must inherit #150's query policy
after the hand merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_014HzBGzjFyN8dXZHbdWmYBj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Release blocker: rollup_1h grain collapse — top()/compare() emit hour averages in peak_aas/p99_aas, fabricating regressions

1 participant