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

ToList is not implemented according to the spec #1133

Open
EvanMachusak opened this issue Mar 3, 2023 · 2 comments
Open

ToList is not implemented according to the spec #1133

EvanMachusak opened this issue Mar 3, 2023 · 2 comments
Labels
Milestone

Comments

@EvanMachusak
Copy link

EvanMachusak commented Mar 3, 2023

According to the ELM spec,

https://cql.hl7.org/04-logicalspecification.html#tolist

The operator is effectively shorthand for "if operand is null then { } else { operand }".

However:
https://github.com/cqframework/clinical_quality_language/blob/d10c4ef801a8e160510b2a868c044dea5bdb85c1/Src/java/engine/src/main/java/org/opencds/cqf/cql/engine/elm/execution/ToListEvaluator.java

When operand is already a list, the ruler simply returns the operand. That logic is on line 12.

This is important as the cql-to-elm translator assumes this behavior. Consider this CQL:


private codesystem "Test": 'https://example.org/fhir/codesystem/test-cs'
private code "Code 1": '1' from "Test"
private code "Code 2": '2' from "Test"
private code "Code 3": '3' from "Test"

define "Test Codes": { "Code 1", "Code 2", "Code 3" }
define "Retrieve with list of codes": [Observation : "Test Codes"]

The translator is generating this ELM for the codes property of the Retrieve:

      "expression" : {
         "type" : "Retrieve",
         "codes" : {
           "type" : "ToList",
           "operand" : {
             "type" : "ExpressionRef",
             "resultTypeSpecifier" : {
               "type" : "ListTypeSpecifier",
               "elementType" : {
                 "type" : "NamedTypeSpecifier",
                 "name" : "{urn:hl7-org:elm-types:r1}Code"
               }
             },
             "locator" : "24:54-24:65",
             "name" : "Test Codes"
           }
         },

Because the ruler ignores the ToList for expressions that are already lists, it codes would end up being expressed as a List<Code>.

If you were to follow the letter of the ELM spec, codes ends up expressed as a List<List<Code>>.

This confounds engines that expect the terminology element of the retrieve statement to be a List<Code> (or a value set, which is implicitly convertible to a List<Code>).

@cmoesel
Copy link
Member

cmoesel commented Mar 14, 2023

IMO, the implementation makes sense -- if a list is passed to ToList, it should return the list as-is rather than increase the dimensions of the list. Which means that my recommendation would be to update the specification. I'd consider this a "technical correction". @brynrhodes - what say you?

@EvanMachusak
Copy link
Author

I can see it either way.

I don't mind whether this is an implementation change or a spec change.

We could tack this into the ELM spec:

When applied to a value that is already a List, this expression returns the existing List.

@brynrhodes brynrhodes added the bug label Mar 28, 2023
@brynrhodes brynrhodes added this to the Maintenance milestone Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants