Skip to content

fix(peewee): retain accepted long events in overlap queries - #197

Open
TimeToBuildBob wants to merge 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/long-event-overlap
Open

TimeToBuildBob wants to merge 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/long-event-overlap

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

Summary

Fixes #194 by removing the lossy timestamp >= start - 24h prefilter. 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 at 2026-10-01T00:00:00Z and query 2026-10-02T12:00Z–13:00Z: unbounded read contains it, bounded read is empty and count is zero.

  • New repository regressions: 6 failed / 33 passed before, 39 passed after.
  • Coverage: 24/48/240-hour events; interior and inclusive boundary hits; non-overlaps; one-sided/unbounded/empty ranges; equivalent timezone offsets; ordinary/tied events; limits/order; unchanged stored content; real overlap query's composite-index plan.
  • Core suite excluding 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.
  • Cross-repo HTTP regressions on aw-server master 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_bucket AQL 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.

Operation Old (lossy) median Correct median
Count 0.68 ms 37.62 ms
Read all matching rows 1.39 ms 46.99 ms
Read with limit 100 1.23 ms 45.70 ms

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.

@greptile-apps

greptile-apps Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[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

  1. P2 Plan test misses sorting ▶

Summary

Removes Peewee's lossy 24-hour timestamp cutoff so reads and counts retain accepted long events that overlap the requested range.

  • Peewee range queries now include overlapping events of any duration.

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

Comment thread tests/test_peewee_long_events.py Resolved
@TimeToBuildBob

TimeToBuildBob commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 AI code review

Removes 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 findings

Confidence 5/5

✅ No findings. The diff looks correct to me on this pass.

Files changed (2) — the diff as I read it
  • aw_datastore/storages/peewee.py — Removes the lower timestamp bound from _where_range(), retaining the event-end overlap predicate and upper timestamp bound.
  • tests/test_peewee_long_events.py — Adds a temporary-database fixture and tests covering long-event range queries, mixed-event ordering, limits, timezone offsets, empty ranges, and SQLite query plans.
Previous review passes
commit score findings engine when
02f5c80426eb 5/5 0 subscription 2026-10-28 23:22 UTC

Reviewed 74946601dbc7 · panel: codex:gpt-6.1-sol · subscription engine · 19s · about this reviewer

Maintainer commands

@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.

Outage coverage: 1-pass, not merge evidence. Subscription review supplied findings while full consensus capacity was unavailable. The consensus merge gate remains unsatisfied.

Git-Session-Id: d8a22e8f-4386-4789-bab9-5ddd743d4b08
Git-Session-Id: d8a22e8f-4386-4789-bab9-5ddd743d4b08
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (1dcab4c) and pushed 7494660. The only conflict was the Linux runner pin: kept master's Ubuntu 22.04 configuration and dropped this branch's redundant Ubuntu 24.04 CI commit. Both product/test commits are unchanged (git range-diff confirms).

Verification after rebase: 395 tests passed, 5 skipped (suite excluding tests/test_flood.py, isolated temporary data/config/cache directories); mypy clean on 37 source files; pinned Ruff and pre-commit hooks passed. The first non-isolated run hit an existing random test-bucket ID collision; the isolated rerun passed without source changes.

GitHub now reports the branch mergeable. New-head CI is running; no merge performed.

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.

Peewee range queries omit accepted events longer than 24 hours

1 participant