Compare DateTimes at the offset the evaluation actually ran at - #145
Open
bryantaustin13 wants to merge 1 commit into
Open
bryantaustin13 wants to merge 1 commit into
bryantaustin13 wants to merge 1 commit into
Conversation
A DateTime always has an offset during evaluation: one written without an offset takes the offset of the evaluation request. Expected outputs are almost always written without one (`@2005-05-10T10:20:30`), while engines must return one, because FHIR requires it on a dateTime with a time. The runner compared the two as text, so a correct result failed whenever an offset was attached, and `Z` never matched `+00:00`. This is the comparison side of #138, on top of the request side (#142): - Two DateTimes with at least an hour are equal when, at the same precision, they denote the same instant. A side without an offset is read at the evaluation offset. Different precisions stay unequal; Dates and Times keep plain comparison. This covers `Z` = `+00:00` (#83) and offset vs no offset (#84), and supersedes #132. - The evaluation offset is the one the evaluation actually ran at: the requested offset if the server honours the timestamp parameter or Timezone header, otherwise the server's own. The start-of-run probe now records the latter as `evaluationRequest.serverTimezoneOffset`, observed from an offset-less DateTime it evaluates. With neither known, offset-less values only equal offset-less values, as before. - A result whose `_valueDateTime`, or Period `_start` / `_end`, carries `time-hasOffset: false` (Using CQL With FHIR) is read as having no offset. - The offset reaches nested values: lists, tuples and interval boundaries. Against HAPI FHIR 8.10.0 / CQL engine 5.3.0 (clinical-reasoning 4.12.0), which ignores both mechanisms and evaluates at -06:00, the full cql-tests suite goes from 1639/170/14/0 to 1665/144/14/0: exactly 26 tests move from fail to pass (22 offset vs no offset, 1 `Z` vs `+00:00`, 3 intervals with DateTime boundaries), and no other record changes status or actual value. 270 unit tests pass (252 before, 18 new). Instants are built with setUTCFullYear, not Date.UTC, which reads years 0-99 as 1900-1999 and would make @0001 equal @1901. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
The comparison side of #138, stacked on #142 (the request side). Closes #83 and #84. Supersedes #132.
Problem
A DateTime always has an offset during evaluation. In the CQL Author's Guide: "If no timezone offset is specified, the timezone offset of the evaluation request timestamp is used." Expected outputs are almost always written without one, but an engine must return one, because FHIR requires an offset on a
dateTimewith a time. The runner compared DateTimes as text, so:#142 sends the evaluation timestamp and offset, defaulting to UTC. On its own that fixes no test, because the comparison is still textual. This PR changes the comparison.
Change
DateTime equality (
src/shared/datetime-comparison.ts):Z=+00:00, and@…T16:20:30Z=@…T10:20:30-06:00.Which offset the evaluation ran at:
timestampparameter or theTimezoneheader (see Send an evaluation request timestamp, defaulting to UTC, with every test #142), it is the test's requested offset: UTC by default, or the test'sevaluationTimezoneOffset/evaluationDateTime.evaluationRequest.serverTimezoneOffset, and comparison uses it. This judges each result against the evaluation that actually produced it, rather than against an offset the server never used. The run log states it, e.g.results are compared at the server's own offset -06:00.time-hasOffset: a result whose_valueDateTime, or Period_start/_end, carriestime-hasOffset: false(Using CQL With FHIR) is read as having no offset. This is @brynrhodes's request from #77 / #119.The evaluation offset is threaded through
resultsEqualinto lists, tuples and interval boundaries. The README section added in #142 now explains the comparison, and the results schema gainsserverTimezoneOffset.Verification
Against HAPI FHIR 8.10.0 / CQL engine 5.3.0 (clinical-reasoning 4.12.0). This server ignores both mechanisms, and the probe reports
serverTimezoneOffset: -06:00. Full cql-tests suite:mainExactly 26 tests move from fail to pass, and no other record changes status or actual value across all 1823:
-06:00, for exampleDateTimeAdd5Seconds,DateTimeMillisecond,HighBoundaryDateTimeMillisecond,ToDateTime3,DateTimeMin,DateTimeMax;Zvs+00:00(ToDateTime6);TestPeriod1,TestPeriod2,DateTimeIntervalTest).All 22 offset cases carry the same
-06:00, including January values when the host's zone is at-07:00. So the engine uses one request-level offset, as CQL specifies, not a zone, which is what makes a single observed server offset sound.270 unit tests pass (252 before, 18 new). They cover:
time-hasOffseton values and Period boundaries;runTestpassing or failing against a server that ignores the request.One bug was caught while testing: instants are built with
setUTCFullYearrather thanDate.UTC, which reads years 0–99 as 1900–1999 and would have made@0001equal@1901.Carried over from #132
#132 is closed in favour of this PR, so its review discussion is reproduced here:
This PR covers #83's case (
ToDateTime6,Zvs+00:00), so it carriesCloses #83, along with #84. Because it is stacked on #142, GitHub closes those issues automatically only if it merges intomain. If it merges intoevaluation-timestamp-and-offsetfirst, #83 and #84 need closing by hand once the change reachesmain.Notes
Zequal+00:00with a narrow string check at the same place inresults-utils.ts. This PR covers that case as part of instant equality, so Equate the Z and +00:00 spellings of UTC when comparing temporal lite… #132 is closed in its favour.dateTime(@2005-05-10T10arrives as@2005-05-10T) — the engine could sendtime-precision, which the runner already honours (Honour the time-precision extension on dateTime, time and Period boundaries #135);expandreturning DateTimes for Dates (expand Interval of Dates is returning intervals of DateTime clinical_quality_language#1770).tsc --noEmitstill reports the two pre-existingrest-routes.tserrors onmain, which Declare the /jobs/:id route parameter type so the server type-checks #139 fixes.🤖 Generated with Claude Code