Fix DateTime initialization and comparison code to deal correctly wit…

…h half hour and fifteen minute timezones (#1259) * Add test to run on Newfoundland TZ machine. * Copy all 4 tests failing on the Newfoundland laptop into the timezone CQL file. * Copy all 4 tests failing on the Newfoundland laptop into the timezone CQL file. * Change Newfoundland tests to to SoftAssert. * Ensure all tests fail in Newfoundland. * Fix wrong test name. * Successfully set timezone within a unit test and replicated the expected failures on an EST/EDT workstation. * Parameterized test with different timezones. * Bugfix that works on standard timezone workstation. * Add parameterized tests for CqlTypesOperatorsTest, which fails on the Newfoundland workstation due a flaw in the bugfix. * Instantiate ZoneId in DateTime from all constructors. Add getter. Add two getNormalized() methods, one of which takes another ZoneId. Pass in the "this" DateTime's zoneId when normalizing the "other" DateTime. Add stub with comments for new DateTimeTest. Lots of TODOs and cleanup needed. Need to test on Newfoundland workstation. * Fix case in test CQL file to fix pipeline failure. Cleanup code in DateTime. * Add tests to new unit test. Leave open the question of how to handle DST. TODO: 2 param getNormalized(). * More tweaks. Try to optimize the performance of DateTime. Still haven't figured out why Newfoundland in non-DST fails. * Comment out experimental code that may impact performance. * Stop using ZoneId and use ZoneOffset instead. This seems to resolve the Newfoundland non DST bug. * Remove all remnants of old ZoneId code. Add more unit tests. * More cleanup.
cqframework · Oct 27, 2023 · 2751734 · 2751734
1 parent 7091010
commit 2751734
Show file tree

Hide file tree

Showing 8 changed files with 677 additions and 200 deletions.
diff --git a/Src/java/engine/src/main/java/org/opencds/cqf/cql/engine/runtime/DateTime.java b/Src/java/engine/src/main/java/org/opencds/cqf/cql/engine/runtime/DateTime.java
@@ -1,5 +1,7 @@
 package org.opencds.cqf.cql.engine.runtime;
 
+import org.opencds.cqf.cql.engine.exception.InvalidDateTime;
+
 import java.math.BigDecimal;
 import java.time.LocalDateTime;
 import java.time.OffsetDateTime;
@@ -8,10 +10,8 @@
 import java.util.Date;
 import java.util.TimeZone;
 
-import org.opencds.cqf.cql.engine.exception.InvalidDateTime;
-import org.opencds.cqf.cql.engine.execution.State;
-
 public class DateTime extends BaseTemporal {
+    private final ZoneOffset zoneOffset;
 
     private OffsetDateTime dateTime;
     public OffsetDateTime getDateTime() {
@@ -40,14 +40,18 @@ public DateTime withPrecision(Precision precision) {
     public DateTime(OffsetDateTime dateTime) {
         setDateTime(dateTime);
         this.precision = Precision.MILLISECOND;
+        zoneOffset = toZoneOffset(dateTime);
     }
 
     public DateTime(OffsetDateTime dateTime, Precision precision) {
         setDateTime(dateTime);
         this.precision = precision;
+        zoneOffset = toZoneOffset(dateTime);
     }
 
     public DateTime(String dateString, ZoneOffset offset) {
+        zoneOffset = offset;
+
         // Handles case when Tz is not complete (T02:04:59.123+01)
         if (dateString.matches("T[0-2]\\d:[0-5]\\d:[0-5]\\d\\.\\d{3}(\\+|-)\\d{2}$")) {
             dateString += ":00";
@@ -98,6 +102,8 @@ public DateTime(BigDecimal offset, int ... dateElements) {
             throw new InvalidDateTime("DateTime must include a year");
         }
 
+        zoneOffset = toZoneOffset(offset);
+
         StringBuilder dateString = new StringBuilder();
         String[] stringElements = TemporalHelper.normalizeDateTimeElements(dateElements);
 
@@ -128,13 +134,16 @@ else if (i == 6) {
         // Otherwise, parse as a LocalDateTime and then interpret that in the evaluation timezone
 
         if (offset != null) {
-            dateString.append(ZoneOffset.ofHoursMinutes(offset.intValue(), new BigDecimal("60").multiply(offset.remainder(BigDecimal.ONE)).intValue()).getId());
+            dateString.append(toZoneOffset(offset).getId());
             setDateTime(OffsetDateTime.parse(dateString.toString()));
         }
         else {
             setDateTime(TemporalHelper.toOffsetDateTime(LocalDateTime.parse(dateString.toString())));
         }
+    }
 
+    public ZoneOffset getZoneOffset() {
+        return zoneOffset;
     }
 
     public DateTime expandPartialMinFromPrecision(Precision thePrecision) {
@@ -204,10 +213,14 @@ public Integer compare(BaseTemporal other, boolean forSort) {
         }
     }
 
-    public OffsetDateTime getNormalized(Precision precision, State c) {
+    public OffsetDateTime getNormalized(Precision precision) {
+        return getNormalized(precision, zoneOffset);
+    }
+
+    public OffsetDateTime getNormalized(Precision precision, ZoneOffset nullableZoneOffset) {
         if (precision.toDateTimeIndex() > Precision.DAY.toDateTimeIndex()) {
-            if (c != null) {
-                return dateTime.atZoneSameInstant(c.getEvaluationZonedDateTime().getZone()).toOffsetDateTime();
+            if (nullableZoneOffset != null) {
+                return dateTime.withOffsetSameInstant(nullableZoneOffset);
             }
 
             return dateTime.atZoneSameInstant(TimeZone.getDefault().toZoneId()).toOffsetDateTime();
@@ -216,18 +229,14 @@ public OffsetDateTime getNormalized(Precision precision, State c) {
         return dateTime;
     }
 
-    public OffsetDateTime getNormalized(Precision precision) {
-        return getNormalized(precision, null);
-    }
-
     @Override
     public Integer compareToPrecision(BaseTemporal other, Precision thePrecision) {
         boolean leftMeetsPrecisionRequirements = this.precision.toDateTimeIndex() >= thePrecision.toDateTimeIndex();
         boolean rightMeetsPrecisionRequirements = other.precision.toDateTimeIndex() >= thePrecision.toDateTimeIndex();
 
         // adjust dates to evaluation offset
         OffsetDateTime leftDateTime = this.getNormalized(thePrecision);
-        OffsetDateTime rightDateTime = ((DateTime) other).getNormalized(thePrecision);
+        OffsetDateTime rightDateTime = ((DateTime) other).getNormalized(thePrecision, getZoneOffset());
 
         if (!leftMeetsPrecisionRequirements || !rightMeetsPrecisionRequirements) {
             thePrecision = Precision.getLowestDateTimePrecision(this.precision, other.precision);
@@ -292,4 +301,19 @@ public static DateTime fromJavaDate(Date date) {
         calendar.setTime(date);
         return new DateTime(OffsetDateTime.ofInstant(calendar.toInstant(), calendar.getTimeZone().toZoneId()), Precision.MILLISECOND);
     }
+
+    private ZoneOffset toZoneOffset(OffsetDateTime offsetDateTime) {
+        return offsetDateTime.getOffset();
+    }
+
+    private ZoneOffset toZoneOffset(BigDecimal offsetAsBigDecimal) {
+        if (offsetAsBigDecimal == null) {
+            return null;
+        }
+
+        return ZoneOffset.ofHoursMinutes(offsetAsBigDecimal.intValue(),
+                new BigDecimal(60)
+                .multiply(offsetAsBigDecimal.remainder(BigDecimal.ONE))
+                        .intValue());
+    }
 }
diff --git a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlMainSuiteTest.java b/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlMainSuiteTest.java
@@ -42,6 +42,27 @@ public void test_all_portable_cql_engine_tests() {
 
     }
 
+    @Test
+    public void test_cql_timezone_tests() {
+        var e = getEngine(testCompilerOptions());
+        // TODO: It'd be interesting to be able to inspect the
+        // possible set of expressions from the CQL engine API
+        // prior to evaluating them all
+
+        var result = e.evaluate(toElmIdentifier("CqlTimeZoneTestSuite"), evalTime);
+
+        for (var entry : result.expressionResults.entrySet()) {
+            if(entry.getKey().toString().startsWith("test")) {
+                if(((ExpressionResult)entry.getValue()).value() != null) {
+                    Assert.assertEquals(
+                            (String) ((ExpressionResult) entry.getValue()).value(),
+                            entry.getKey().toString().replaceAll("test_", "") + " TEST PASSED"
+                    );
+                }
+            }
+        }
+    }
+
    protected CqlCompilerOptions testCompilerOptions() {
         var options = CqlCompilerOptions.defaultOptions();
         // This test suite contains some definitions that use features that are usually

diff --git a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlTestBase.java b/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlTestBase.java
@@ -5,8 +5,24 @@
 import org.cqframework.cql.cql2elm.*;
 import org.hl7.elm.r1.Library;
 import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
 
 public class CqlTestBase {
+    static final String NORTH_AMERICA_MOUNTAIN = "America/Denver"; // This is the baseline:  Normal hour on the hour timezone
+    static final String NEWFOUNDLAND = "America/St_Johns";
+    static final String INDIA = "Asia/Kolkata";
+    static final String AUSTRALIA_NORTHERN_TERRITORY = "Australia/Darwin";
+    static final String AUSTRALIA_EUCLA= "Australia/Eucla";
+    static final String AUSTRALIA_BROKEN_HILL = "Australia/Broken_Hill";
+    static final String AUSTRALIA_LORD_HOWE = "Australia/Lord_Howe";
+    static final String AUSTRALIA_SOUTH = "Australia/Adelaide";
+    static final String INDIAN_COCOS = "Indian/Cocos";
+    static final String PACIFIC_CHATHAM = "Pacific/Chatham";
+
+    @DataProvider
+    static Object[][] timezones() {
+        return new Object[][] {{NORTH_AMERICA_MOUNTAIN},{NEWFOUNDLAND},{INDIA},{AUSTRALIA_NORTHERN_TERRITORY},{AUSTRALIA_EUCLA},{AUSTRALIA_BROKEN_HILL},{AUSTRALIA_LORD_HOWE},{AUSTRALIA_SOUTH},{INDIAN_COCOS},{PACIFIC_CHATHAM}};
+    }
 
     private static ModelManager modelManager;
     protected static ModelManager getModelManager() {

diff --git a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlTimezoneTests.java b/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlTimezoneTests.java
@@ -0,0 +1,42 @@
+package org.opencds.cqf.cql.engine.execution;
+
+import org.hl7.elm.r1.VersionedIdentifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.*;
+import org.testng.asserts.SoftAssert;
+
+import java.util.TimeZone;
+
+@SuppressWarnings("removal")
+public class CqlTimezoneTests extends CqlTestBase {
+    private static final Logger logger = LoggerFactory.getLogger(CqlTimezoneTests.class);
+
+    private static final VersionedIdentifier library = new VersionedIdentifier().withId("CqlTimezoneTests");
+
+    @Test(dataProvider = "timezones")
+    public void testExpressionsProblematicForWeirdTimezones(String timezone) {
+        final String oldTz = System.getProperty("user.timezone");
+        // This is the ONLY thing that will work.  System.setProperty() and -Duser.timezone do NOT work
+        TimeZone.setDefault(TimeZone.getTimeZone(timezone));
+
+        try {
+            final SoftAssert softAssert = new SoftAssert();
+
+            evaluateExpression("After_SameHour", false, softAssert);
+            evaluateExpression("SameAs_SameHour", true, softAssert);
+            evaluateExpression("SameOrAfter_HourBefore", false, softAssert);
+            evaluateExpression("SameOrBefore_SameHour", true, softAssert);
+
+            softAssert.assertAll();
+        } finally {
+            TimeZone.setDefault(TimeZone.getTimeZone(oldTz));
+        }
+    }
+
+    private void evaluateExpression(String functionName, boolean expectedResult, SoftAssert softAssert) {
+        Object result = engine.expression(library, functionName).value();
+        softAssert.assertEquals(result, expectedResult, functionName);
+    }
+
+}