Seemingly inconsistent null handling in List Operators tests

[dehall]

We've found 2 tests in CqlListOperatorsTest that appear to have inconsistent handling of nulls:

1. ProperContains9: { 'a', null } properly includes 'a' ==> null
   

   cql-tests/tests/cql/CqlListOperatorsTest.xml

   Lines 899 to 903 in 0666708

   	<test name="ProperContains9" version="1.0">
   	<capability code="list-operators" />
   	<expression>{ 'a', null } properly includes 'a'</expression>
   	<output>null</output>
   	</test>

This one we believe is correct. From the ProperContains spec,

this operator returns true if the given element is in the list, and it is not the only element in the list, using equality semantics, with the exception that null elements are considered equal.

(Note we interpret "not the only element" to mean "not the only unique element" here, and a pseudocode for how we would implement ProperContains is list.any?(e -> givenElement == e) and list.any?(e -> givenElement != e) )

In this example, the given element 'a' is in the list, but by the usual equality semantics, null == 'a' evaluates to null, and so evaluating "is it the only element in the list?" must evaluate to null as well and that bubbles up into the overall result.

and

2. ContainsNullFirst: { null, 'b', 'c' } contains 'a' ==> false
   

   cql-tests/tests/cql/CqlListOperatorsTest.xml

   Lines 70 to 74 in 0666708

   	<test name="ContainsNullFirst" version="1.0">
   	<capability code="list-operators" />
   	<expression>{ null, 'b', 'c' } contains 'a'</expression>
   	<output>false</output>
   	</test>

This one we believe is incorrect, and the expected output should instead be null

From the Contains spec,

this operator returns true if the given element is in the list, using equality semantics, with the exception that null elements are considered equal.

In this example, again we have an equality comparison that results in null: null == 'a', none of the comparisons result in true, and so as before this should bubble up to be null for the overall result.

Are we maybe missing something? Or possibly should we be interpreting the null-equality semantics differently in one of these cases?

[cmoesel]

@brynrhodes - I'm not 100% sure of which is correct above (although I tend to agree w/ Dylan's interpretation), but in either case, there is probably opportunity to clarify the spec in this area. It's apparently ambiguous enough that we have test cases expecting different results based on conflicting interpretations.

[brynrhodes]

This is a great find, and in digging through these, I found several more issues with the examples in the spec, and logged the following technical corrections to address those:

https://jira.hl7.org/browse/FHIR-57721
https://jira.hl7.org/browse/FHIR-57723

I think those are non-controversial corrections to the examples and explanatory text (feel free to comment if you feel differently), but they don't address the questions being raised with these tests specifically.

I think the answer hinges on whether list membership tests should distinguish between a null element, and a null result of an equality test while looking for a non-null element. To resolve this, looking at the includes(List<T>, T) overload (synonym for contains),

The contains operator for lists returns true if the given element is in the list using equality semantics, with the exception that null elements are considered equal.

To me, the implication is that if we don't know for sure the element is in the list, the result is false (i.e. it's if and only if), and that's consistent with the intent of being able to identify null elements in membership tests.

The tests are inconsistent though, with some having the true/false only interpretation:

<test name="ContainsNullFirst" version="1.0">
    <capability code="list-operators" />
    <expression>{ null, 'b', 'c' } contains 'a'</expression>
    <output>false</output>
</test>

And others having a "null is possible" interpretation:

<test name="ProperContains9" version="1.0">
    <capability code="list-operators" />
    <expression>{ 'a', null } properly includes 'a'</expression>
    <output>null</output>
</test>

and

<test name="ProperContainsTimeNull" version="1.0">
    <capability code="list-operators" />
    <expression>{ @T15:59:59.999, @T20:59:59.999, @T20:59:49.999 } properly includes @T15:59:59</expression>
    <output>null</output>
</test>

I think the simplest implementation is to say:

define function Contains(AList List<T>, AElement T):
  exists (
    AList Item
      where (AElement is null and Item is null)
        or (AElement = Item)
  )

But on this interpretation, several of the tests are incorrect for all of the list membership operators.

I think this is the first thing we need to get consensus on.

Then can talk about the "proper" membership operators, which I read as, the membership operators, plus the list is strictly larger (i.e. has more elements than just the one being searched). There isn't any language that I can find in any of these operators that explicitly says they have set semantics (though I certainly have sympathy for that view, and it is definitely implied by the use of set terms like "proper" and "strictly larger", but the intent of having list semantics is that they are more general, because I can always get to the set semantics by invoking a distinct, but I could never get to the list semantics if I only had the set semantics.

So I'm inclined to propose that:

1. We clarify that the list membership operators will return true or false (i.e. we add "otherwise the result is false" to clarify)
2. We clarify that the proper list operators (membership and otherwise) have list, not set, semantics

And then propose that the tests be updated accordingly.

Thoughts?

[dehall]

All sounds great, thanks! I had a brief discussion with @cmoesel this morning and we’re good with both of your proposed clarifications. Last week we were trying to determine whether the spec implied set or list semantics for the proper list operators, and a deciding factor was which way the majority of the tests indicated. For example this one as of today suggests a pretty clear expectation of set semantics:

            <test name="ProperContains7" version="1.0">
                  <capability code="list-operators" />
                  <expression>{ 'a', 'a' } properly includes 'a'</expression>
                  <output>false</output>
            </test>

But really, we don’t have a strong preference which way the behavior “should” be, as long as the behavior is consistent and fully specified we’re happy.

[brynrhodes]

Perfect, I've logged a tracker here: https://jira.hl7.org/browse/FHIR-57746

I'll propose a PR that updates all the relevant tests based on this proposed disposition and raise it on Zulip for review.

[brynrhodes]

PR to address this issue: #119