Remove incorrect interval with null boundaries test

[brynrhodes]

The test CqlIntervalOperatorsTest.Except.NullInterval is incorrect:

https://github.com/cqframework/cql-tests/blob/main/tests/cql/CqlIntervalOperatorsTest.xml#L651

It states that Interval[null, null] should result in null but this is not correct, and most of the engines fail this incorrect test.

The spec is clear that an interval with null boundaries is allowed, and especially in the case of null closed boundaries, the semantics are very different from just collapsing to a null.

This test should be removed.

[brynrhodes]

Note that this test exists in the spec as far back as the github history in HL7 goes. In trying to identify how this test was introduced, it appears to be related to a comment in the STU4 ballot, which was submitted in response to a CQLIT ticket:

https://oncprojectracking.healthit.gov/support/browse/CQLIT-162

The ticket was discussed as a topic on Cooking with CQL Session 37, and the conclusion of that discussion was:

Guidance in this case and others like it is to consider an interval with both endpoints null,
regardless of whether the boundaries are closed or open, as a null interval (i.e. the
interval collapses to a null)

A comment on the specification will be submitted to add this interpretation of an interval
with both endpoints null.

Source

That comment was submitted as part of the 2019 May CQL STU4 Ballot, it ended up being comment #35 in that specification, and suggested:

If an interval has both boundaries null and closed, the current semantics indicate that this is the interval from the beginning of time to the end of time. However, this results in surprising behavior when performing comparisons that involve building an interval from a single value (such as the interval from 30 days before to 30 days after the documentation time of an event). If the point used to build the interval is null, this results in an effectively infinite interval, and all comparisons return true, resulting the opposite of the intent. The "null" interval should be considered a null like any other throughout CQL.

However, reconciliation identified the root cause of the issue in that case being that the "within" translation phrases were incorrectly translating when the point argument to the within phrase was null:

This is an error in the specification. Specifically, the timing phrase translations in 5.5.3.2 and 5.5.3.3. The problem is that these translations are constructing intervals out of expressions that may result in null, and whenever an inclusive boundary is used with a null, the resulting translation incorrectly resolves to the beginning or end of time, rather than the unknown that was used to construct the interval. The translation semantics either need to add an explicit check for null, or use predecessor and successor functions so that an exclusive boundary can always be used. The former approach is preferred as simpler and more correct in the presence of variable precision values.

The comment was found Not persuasive with mod and the following change was actually applied to the spec:

Change the timing phrase translations in the specification to account for the possibility that the expression used to construct an interval as part of the translation may be null by adding a null check to the translation.

Source

The change log shows this change applied as part of STU4 ballot reconciliation:

STU4B#35: Changed timing phrase translations to add null check when the translation constructs an interval from a nullable element (Substantive)

Source

So, this test seems to have been added as part of the proposed addition, but it was never removed as part of the final disposition.

[bryantaustin13]

PR #109 fixes this

[cmoesel]

This is not the only test that incorrectly expects Interval[null, null] to be null. For example, also note this test that says Interval[null, null] overlaps Interval[1, 10] should be null. I'd suggest searching the whole suite of test cases for Interval[null, null] and checking each occurrence for correctness.

[brynrhodes]

Agreed, @cmoesel , I reviewed all the tests that reference "max interval" Interval[null, null] and most of them were incorrect. I have updated the tests, as well as added some more edge tests for the "unknown interval" Interval(null, null)

#116

If you get a chance, I'd love feedback on whether I got the proposed results for each test correct :)

[cmoesel]

Thanks for looking at this, @brynrhodes. I think all of your proposed results are technically correct according to spec.

The only one that could have a more nuanced answer is Interval(null, null) union Interval[1, 10], which you could say represents Interval[Uncertainty<minimum Integer,1>, Uncertainty<10, maximum Integer>] -- but there's no way to express that literally in CQL, and it might not be technically correct based on the strictest reading of the spec.

BTW -- I predict some more talk of Intervals w/ null boundaries in your future. I think the FLAME team is planning to propose a related topic in an upcoming Friday call.