Fix date-only range end dropping the final day - #296
Fix date-only range end dropping the final day#296Shadow_Lu (LuShadowX) wants to merge 1 commit into
Conversation
|
Hi Shadow_Lu (@LuShadowX) The resolution of us was a half-open interval [start, stop) — not an inclusive end padded to 23:59:59.999999. That convention is now codified in email_import.py:274-281 (email_matches_date_filter, docstring: "The range is half-open: [start_date, stop_date)"). So your PR #296's approach (inclusive end + magic 23:59:59.999999) is exactly the ambiguity-prone pattern we explicitly moved away from elsewhere in this codebase — it reintroduces the "does end-of-day mean 23:59:59.999999 or is it just as confusing as 'midnight'?" question. So please understand that i reject this PR. I will however add this information to AGENTS.md so that we agree on halfopen intervals. Thanks again. |
|
Hi Shadow_Lu (@LuShadowX) — I owe you an apology. I rejected this PR over the approach (inclusive end padded to Worse, I pointed you at the half-open
I've now implemented the fix in #309, using the half-open form of your fix: a date-only stop rolls forward to midnight of the next day, so If you'd rather land this yourself, I'm happy to close #309 and reopen this PR for you to push the half-open version to |
…op)) A search time range whose stop_date carries no time compiled to that day's midnight, so "Jan 1 to Jan 5" became [Jan 1 00:00, Jan 5 00:00) and every message on Jan 5 was silently excluded. Reported in PR microsoft#296. Both timestamp index backends already filter half-open (get_in_range: "End is exclusive"; SQLite: start_timestamp >= ? AND start_timestamp < ?), so the fix is to make the compiled stop the exclusive bound the storage layer expects: for a bare date, midnight of the following day. A stop with an explicit time is already exclusive and is kept as-is. This follows the [start, stop) convention agreed in PR microsoft#198 and documented in AGENTS.md, rather than padding an inclusive end to 23:59:59.999999. Also fixes two boundary inconsistencies this uncovered: - DateRange documented and implemented its end as inclusive (start <= dt <= end) while both storage backends excluded it. It is now documented and implemented as half-open, so the two scope-filtering paths agree: with a timestamp index (lookup_range) and without one (get_text_range_for_date_range, which uses __contains__). - get_time_range_for_conversation builds an inclusive-looking end from the last message's timestamp; documented as prompt-display only, not for index lookups. get_enclosing_date_range_for_text_range already used the exclusive end ordinal's timestamp, so it needed no change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for coming back to it — no hard feelings, and the half-open form is the better fix. Padding to I'd like to take you up on the offer. If you close #309 and reopen this, I'll push the half-open version to Leaving |
|
Pushed the half-open version to |
…op)) (#309) ## The bug `date_range_from_datetime_range` compiled a `stop_date` with no time to that day's midnight, so a query like "Jan 1 to Jan 5" became `[Jan 1 00:00, Jan 5 00:00)` and every message on Jan 5 was silently dropped. Reproduced on `main` before the fix: ``` compiled DateRange: DateRange(2023-01-01 00:00+00:00, 2023-01-05 00:00+00:00) index hits: ['2023-01-01T00:00:00+00:00', '2023-01-04T09:30:00+00:00'] # dropped: 2023-01-05T00:00, 2023-01-05T09:30, 2023-01-05T23:59 ``` This is the bug reported by @LuShadowX in #296 — credit for finding it goes to them. That PR fixed it by padding the (then inclusive) end to `23:59:59.999999`; this PR fixes it with the half-open `[start, stop)` convention agreed in #198 and documented in AGENTS.md instead. ## The fix Three source edits 1. searchlang.py — new exclusive_stop_from_date_time(); a stop_date with no time now rolls to next-day midnight. One if, plus docstrings. 2. interfaces_core.py — DateRange.end comment inclusive → exclusive, and __contains__ <= → <. Plus a docstring. 3. convutils.py — docstring only, no code. Tests: one regression test for the bug, unit tests for the new helper, two DateRange.__contains__ tests, and test_start_and_stop updated to expect Jan 1 instead of Dec 31. Full `make` is green: isort/black clean, pyright clean, 756 tests pass, build OK. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
When a search time range has a stop date with no time, the compiled
DateRangeend lands at midnight (00:00:00). ButDateRange.endis inclusive, so a query like "Jan 1 to Jan 5" producesend = 2023-01-05 00:00:00, and anything happening on Jan 5 after midnight falls outside the range — the whole final day is effectively dropped.The TypeScript original uses two helpers:
toStartDate(midnight for a date-only value) for the start, andtoStopDate(end-of-day,23:59:59.999) for the stop. The Python port used the start-of-day conversion for both, so the end-of-day behavior was lost.This adds a
stop_datetime_from_date_timehelper mirroringtoStopDate: a date-only stop now covers the whole day (23:59:59.999999), while an explicit time is still honored as-is. Added tests for both cases; the full offline test suite passes.