Repository navigation
fix(peewee): retain accepted long events in overlap queries - #197
TimeToBuildBob wants to merge 2 commits into
Conversation
[Medium impact] The change reaches Peewee-backed event range reads and counts. The fix appears safe to merge, with a non-blocking improvement to the query-plan test. Findings
SummaryRemoves Peewee's lossy 24-hour timestamp cutoff so reads and counts retain accepted long events that overlap the requested range.
Acknowledgment: TimeToBuildBob explicitly accepts slower bounded reads and counts and defers index or schema changes to keep this fix focused. That trade-off is not reported as an issue. Verification used source inspection; no tests or benchmarks were run during this review. Reviews (1) · Last reviewed commit: "fix(peewee): retain accepted long events..." · Reviewed by Greptile |
🤖 AI code reviewRemoves the 24-hour timestamp prefilter from PeeweeStorage._where_range() and drops the unused timedelta import. Adds tests for long-event inclusion, counts, clipping, ordering, limits, timezone offsets, stored values, and the overlap query's index plan. Safe to merge — no P0/P1 findingsConfidence 5/5 ✅ No findings. The diff looks correct to me on this pass. Files changed (2) — the diff as I read it
Previous review passes
Reviewed Maintainer commands
|
Git-Session-Id: d8a22e8f-4386-4789-bab9-5ddd743d4b08
Git-Session-Id: d8a22e8f-4386-4789-bab9-5ddd743d4b08
02f5c80 to
7494660
Compare
|
Rebased onto current master ( Verification after rebase: 395 tests passed, 5 skipped (suite excluding GitHub now reports the branch mergeable. New-head CI is running; no merge performed. |
Summary
Fixes #194 by removing the lossy
timestamp >= start - 24hprefilter. Ingestion accepts longer events, so that bound silently discarded valid overlaps in reads and counts. The existing inclusive end-overlap predicate, UTC normalization, clipping, sorting and limits remain unchanged. No duration cap, schema/index migration or stored-history rewrite.Reproduction and verification
On fetched master
c7821899, insert a 48-hour event at2026-10-01T00:00:00Zand query2026-10-02T12:00Z–13:00Z: unbounded read contains it, bounded read is empty and count is zero.tests/test_flood.py: 385 passed, 5 skipped. Existing Peewee index/query-plan tests included.make typecheck: 37 source files clean. Pinned Ruff and hook-enabled commit clean.994ea988: 4 failed before / 4 passed after for bounded GET/count, CSV row and AQL sum/count. Server local test slice: 231 passed, mypy 32 files clean, with this core tree on PYTHONPATH. No real listener or user history touched. Server regression adoption is tracked in Guard bounded REST, CSV and AQL reads against long-event omission aw-server#213.Peewee already clips returned event copies, including
query_bucketAQL results: the interior result remains one hour, not the entire 48 hours. This repair changes inclusion only.Measured performance trade-off
Correctness has a real cost. Disposable synthetic SQLite DB: 100,000 ordinary 30-second events, one per minute; one-hour range near the end of history; warm alternating runs, 9 measured repetitions. Both predicates return 61 rows.
Both plans use
eventmodel_bucket_id_timestamp; the corrected plan only has the upper timestamp bound and must evaluate earlier rows for possible long overlaps. These are warm synthetic measurements, not a production latency estimate. Retaining the old cutoff, imposing a new ingestion limit, or introducing endtime indexes/migrations would either preserve data loss or widen this bounded fix. Any subsequent optimization must retain arbitrary accepted-duration inclusion.