Fix DateTime initialization to always use offset from State and never from null #1269

lukedegruchy · 2023-11-01T19:20:26Z

Ensure that no DateTime constructor accepts a null offset of any kind
If no explicit offset is provided, obtain one from the State evaluation time
Remove all logic that considered null offsets
Never consider timezones or ZoneIds: The requirement is to always use the same offset even across DST changes
Fix and/or enhance tests to mix up timezones/offsets and evaluation/now timestamps
Hack: Always pass ISO formatted timestamp Strings with offsets to DateTimeType so as to sidestep any default timezone logic when converting DateTimes
Explicitly test the CQL expressions in the linked ticket

Closes #863

…peline.

…timezones and default offset dates.

…tead rely on either the default offsetdatetime.

…s and timezones.

…tate whenever possible. Fix most units with non-null offsets. There are still test failures to investigate. Lots of cleanup needs to be done afterward.

…other machine on a DST date.

…est failures on machine set to DST date.

…val-inclusion

codecov · 2023-11-19T19:14:47Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (2985f47) 0.00% compared to head (bc218b2) 61.36%.

❗ Current head bc218b2 differs from pull request most recent head f2ef8fa. Consider uploading reports for the commit f2ef8fa to get more accurate results

Files	Patch %	Lines
...a/org/opencds/cqf/cql/engine/runtime/DateTime.java	45.45%	12 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #1269       +/-   ##
=============================================
+ Coverage          0   61.36%   +61.36%     
- Complexity        0     6145     +6145     
=============================================
  Files             0      469      +469     
  Lines             0    23651    +23651     
  Branches          0     4566     +4566     
=============================================
+ Hits              0    14514    +14514     
- Misses            0     7028     +7028     
- Partials          0     2109     +2109

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JPercival

approved, contingent upon adding a few test cases to get up to 80% coverage for DateTime. I'm working on that.

c-schuler

Nice work. I have a couple of notes, but am willing to approve. I would like to see the fromJavaDate function restored unless there is a good reason for its removal.

c-schuler · 2023-11-28T18:58:14Z

...ngine-fhir/src/test/java/org/opencds/cqf/cql/engine/fhir/converter/R4TypeConverterTests.java

-import java.util.Iterator;
-import java.util.List;
-import java.util.TimeZone;
+import java.time.*;


Just noting the use of wildcard imports here, which is not best practice.

c-schuler · 2023-11-28T18:59:08Z

Src/java/engine/src/main/java/org/opencds/cqf/cql/engine/runtime/DateTime.java

@@ -296,10 +294,8 @@ public Date toJavaDate() {
        return java.util.Date.from(dateTime.toInstant());
    }

-    public static DateTime fromJavaDate(Date date) {


This is a very useful function that is used in other projects. I would like to know why it has been removed.

The IDE said it was unused and it's my instinct to clean up dead code.

Whoever is maintaining this PR feel free to add it back.

I did a quick review of cqf-tooling and clinical-reasoning, this function is used when parsing the input reporting period. Since it's only used one place I'm fine either way.

First commit.

09b2ac7

JPercival mentioned this pull request Nov 1, 2023

Fixes Measure Evaluate Start and End times cqframework/clinical-reasoning#362

Closed

lukedegruchy added 2 commits November 1, 2023 16:50

More changes with experiments.

85f78e2

Small change to test.

805636c

lukedegruchy changed the title ~~First commit.~~ DateTime Interval fix Nov 10, 2023

lukedegruchy added 22 commits November 10, 2023 16:55

Merge branch 'master' into 863-date-time-interval-inclusion

53fa40c

Add logging to try to figure out why the test passes on the github pi…

8790559

…peline.

More logging.

d5b2b65

Even more logging.

5921f28

Refactor DateTime constructor code to enable unit tests using custom …

d207054

…timezones and default offset dates.

Get rid of all known code that relies on the default timezone and ins…

7053ffe

…tead rely on either the default offsetdatetime.

Run CqlInternalTypeRepresentationSuiteTest with multiple current time…

10c5ad5

…s and timezones.

Throw Exceptions for null offsets in DateTime. Pass in offsets from S…

a39a75e

…tate whenever possible. Fix most units with non-null offsets. There are still test failures to investigate. Lots of cleanup needs to be done afterward.

All tests pass. Lots of code and TODOs to cleanup. Need to test on an…

3f8f114

…other machine on a DST date.

Add DST and non-DST "now" logic to TypeConverter tests to replicate t…

c65b4f1

…est failures on machine set to DST date.

Try a fix for ***TypeConverter tests that I'm not sure about...

2619dd9

Merge remote-tracking branch 'origin/master' into 863-date-time-inter…

2f7303c

…val-inclusion

Fix one use case for DateTimeType conversion that failed on Nov 1st.

04c08fb

Try latest fix for fhir date time conversion.

af236ea

Ensure tests work on Nov 1st.

fb71a94

Start cleaning up TODOs and commented out and abandoned code.

f3e5a59

Enhanced some tests and clean up more TODOs.

7a0b950

Enhance more tests.

158fddf

Enhance even more tests.

efbb68f

Enhance all TypeConverter tests.

2395037

More cleanup and refactoring and removing TODOs.

79267c6

Last set of tweaks to simplify the implementation.

6ee8449

lukedegruchy changed the title ~~DateTime Interval fix~~ Fix DateTime initialization to always use offset from State and never from null Nov 16, 2023

Merge branch 'master' into 863-date-time-interval-inclusion

ff8d757

lukedegruchy marked this pull request as ready for review November 16, 2023 19:33

Merge branch 'master' into 863-date-time-interval-inclusion

bc218b2

vitorpamplona mentioned this pull request Nov 27, 2023

Upgrade sdk libraries to latest versions opensrp/fhircore#2825

Merged

11 tasks

JPercival requested review from brynrhodes and barhodes November 27, 2023 17:11

JPercival approved these changes Nov 28, 2023

View reviewed changes

JPercival added 2 commits November 28, 2023 09:26

Merge branch 'master' into 863-date-time-interval-inclusion

cee5894

Add tests for equals

f2ef8fa

c-schuler approved these changes Nov 28, 2023

View reviewed changes

JPercival merged commit 75e55e9 into master Nov 28, 2023
2 checks passed

JPercival deleted the 863-date-time-interval-inclusion branch November 28, 2023 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DateTime initialization to always use offset from State and never from null #1269

Fix DateTime initialization to always use offset from State and never from null #1269

lukedegruchy commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 19, 2023 •

edited

Loading

JPercival left a comment

c-schuler left a comment

c-schuler Nov 28, 2023

c-schuler Nov 28, 2023

lukedegruchy Nov 28, 2023 •

edited

Loading

JPercival Nov 28, 2023 •

edited

Loading

Fix DateTime initialization to always use offset from State and never from null #1269

Fix DateTime initialization to always use offset from State and never from null #1269

Conversation

lukedegruchy commented Nov 1, 2023 • edited Loading

codecov bot commented Nov 19, 2023 • edited Loading

Codecov Report

JPercival left a comment

Choose a reason for hiding this comment

c-schuler left a comment

Choose a reason for hiding this comment

c-schuler Nov 28, 2023

Choose a reason for hiding this comment

c-schuler Nov 28, 2023

Choose a reason for hiding this comment

lukedegruchy Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

JPercival Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

lukedegruchy commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 19, 2023 •

edited

Loading

lukedegruchy Nov 28, 2023 •

edited

Loading

JPercival Nov 28, 2023 •

edited

Loading