Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DateTime initialization and comparison code to deal correctly with half hour and fifteen minute timezones #1259

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d3f8ca3
Add test to run on Newfoundland TZ machine.
lukedegruchy Oct 24, 2023
694b0bf
Copy all 4 tests failing on the Newfoundland laptop into the timezone…
lukedegruchy Oct 24, 2023
7581383
Copy all 4 tests failing on the Newfoundland laptop into the timezone…
lukedegruchy Oct 25, 2023
50bca1b
Change Newfoundland tests to to SoftAssert.
lukedegruchy Oct 25, 2023
bd84511
Ensure all tests fail in Newfoundland.
lukedegruchy Oct 25, 2023
6cd65d6
Fix wrong test name.
lukedegruchy Oct 25, 2023
7bc82a6
Successfully set timezone within a unit test and replicated the expec…
lukedegruchy Oct 25, 2023
682e04e
Parameterized test with different timezones.
lukedegruchy Oct 25, 2023
b715e61
Bugfix that works on standard timezone workstation.
lukedegruchy Oct 25, 2023
e03fe16
Add parameterized tests for CqlTypesOperatorsTest, which fails on the…
lukedegruchy Oct 25, 2023
221220d
Instantiate ZoneId in DateTime from all constructors. Add getter. A…
lukedegruchy Oct 26, 2023
4ed57ff
Fix case in test CQL file to fix pipeline failure. Cleanup code in D…
lukedegruchy Oct 26, 2023
40afa1d
Add tests to new unit test. Leave open the question of how to handle…
lukedegruchy Oct 26, 2023
55a136e
More tweaks. Try to optimize the performance of DateTime. Still hav…
lukedegruchy Oct 27, 2023
b0389cb
Comment out experimental code that may impact performance.
lukedegruchy Oct 27, 2023
bac08ff
Stop using ZoneId and use ZoneOffset instead. This seems to resolve …
lukedegruchy Oct 27, 2023
82ad161
Remove all remnants of old ZoneId code. Add more unit tests.
lukedegruchy Oct 27, 2023
a97967f
Merge branch 'master' into 1222-build-failure-half-hour-timezone-ex-n…
lukedegruchy Oct 27, 2023
e80fdab
More cleanup.
lukedegruchy Oct 27, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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() {
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand Down Expand Up @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}

}
Loading