fix(search): normalize sqlite date comparisons across precisions - #463
Open
angela-helios wants to merge 2 commits into
Open
fix(search): normalize sqlite date comparisons across precisions#463angela-helios wants to merge 2 commits into
angela-helios wants to merge 2 commits into
Conversation
Stored value_date keeps the resource's own precision ('1995-10-02',
'2016-01-23T13:07:42-04:00') while search bounds were full datetimes, and
SQLite compares TEXT — lexicographically '1995-10-02' sorts before
'1995-10-02T00:00:00', so a day never fell inside its own range: eq
matched nothing, ge excluded the named day, le/lt leaked the next
midnight in. Full-precision values built the impossible range
'>= X AND < X'.
One shared date_condition() now serves the parameter handler, the chain
builder (previously a raw text '=' regardless of precision), and the
_filter parser: both sides go through datetime(), which normalizes
partial dates and folds timezone offsets to UTC, and the upper bound of
a partial value is derived in SQL ('+1 day') so a single bind parameter
serves both ends — which also fixes the latent multi-value numbering
collision where eq consumed two parameter slots while the caller
advanced by one.
Postgres parses dates into real timestamps on its own path; the ES and
Mongo handlers are unverified and covered by the #448 backend legs.
Closes #456
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The codecov patch gap was real: the ne/sa/eb arms, the full-precision single-bound comparisons, the ap windows at finer precisions, the aliased-column pass-through, the chained-terminal delegate, and the _filter date branch had no direct coverage.
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #456
Root cause:
value_datestores the resource's own precision (1995-10-02,2016-01-23T13:07:42-04:00) while query bounds were full datetimes — and SQLite compares TEXT, where'1995-10-02' < '1995-10-02T00:00:00'(the shorter string sorts first). Every boundary misbehaved exactly as the issue table shows, and full-precisioneqbuilt the unsatisfiable>= X AND < X.One shared
date_condition()now serves all three date-comparison sites — the parameter handler, the chain builder (which compared with a raw text=regardless of precision), and the_filterparser: both sides go throughdatetime()(normalizes partial dates, folds timezone offsets to UTC), and partial-precision upper bounds are derived in SQL ('+1 day') so one bind parameter serves both ends. That single-parameter shape also fixes a latent numbering collision: multi-value dateeqconsumed two?Nslots while the caller advanced the offset by one.Verified
patient.birthdate).birthdate=1995now returns 1 — the old "correct" 2 wrongly included a 1996-01-01 birth via the text-compare leak).search_integration105/105; persistence lib 771 tests green.Scope note
SQLite only. Postgres parses dates into real timestamps on a separate path (
parse_date_value) and needs no change; the Elasticsearch and Mongo date handlers are unverified — the #448 backend legs will tell, and findings there get their own issues.