projecttacoma / fqm-execution

fqm-execution is a library that allows users to calculate FHIR-based electronic Clinical Quality Measures (eCQMs) and retrieve the results in a variety of formats
https://projecttacoma.github.io/fqm-execution/
Apache License 2.0
18 stars 6 forks source link

e.union is not a function #172

Closed srankins closed 1 year ago

srankins commented 1 year ago

Good afternoon. The following error is being received from fqm-execution:

e.union is not a function

The measure bundle is available here.

The patient bundle is here.

This is being run against RC2 release.

I'm assuming this is bubbling up from cql-execution, but I thought I would start here. Bug or is this anticipated behavior?

mgramigna commented 1 year ago

Hi Stan,

This is likely not a bug with fqm-execution, and is either a problem with the ELM (which we've seen before in the ecqm-content-qicore repos), or an issue with cql-execution, or a combination of those two

After a very brief test, it seems like something with the SDE expressions in that measure is causing this. I will investigate more and see if I can identify a cause.

srankins commented 1 year ago

Thank you @mgramigna. This was based on an issue a user raised. If it ends up being an issue with cql-execution, I can post something there. But that update would have to be consumed by fqm-execution in order to bring it into MADiE, so I thought I would start here.

mgramigna commented 1 year ago

Yeah, makes total sense. I'll give it a good look tomorrow and we can identify next steps. Thanks!

p9g commented 1 year ago

This may not matter, but the measure bundle is missing .id for the Bundle and the Measure.

mgramigna commented 1 year ago

@p9g That shouldn't affect calculation for the fqm-execution since it doesn't rely on resource IDs

mgramigna commented 1 year ago

@srankins sorry for the late response. I did some digging, and have confirmed my above suspicion.

TL;DR: The translator + cql-execution aren't playing nicely for the SDE expressions when no SDE extensions are present on the patient resource

This issue is raised when executing SDE expressions. Specifically, I think there's some weirdness going on with lists, Codings, and how they're handled in both the translator and the execution engine. See the following differences between SDE Race/Ethnicity expressions in FHIR-based CQL vs. QICore-based CQL

From SDE Library using FHIR 4.0.1:

define "SDE Ethnicity":
  (flatten (
    Patient.extension Extension
      where Extension.url = 'http://hl7.org/fhir/us/core/StructureDefinition/us-core-ethnicity'
        return Extension.extension
  )) E
    where E.url = 'ombCategory'
      or E.url = 'detailed'
    return E.value as Coding

define "SDE Race":
  (flatten (
    Patient.extension Extension
      where Extension.url = 'http://hl7.org/fhir/us/core/StructureDefinition/us-core-race'
        return Extension.extension
  )) E
    where E.url = 'ombCategory'
      or E.url = 'detailed'
    return E.value as Coding

From SDE Library using QICore 4.1.1:

define "SDE Ethnicity":
  Patient.ethnicity E
    return Concept {
      codes: { E.ombCategory } union E.detailed,
      display: E.text
    }

define "SDE Race":
  Patient.race R
    return Concept {
      codes: R.ombCategory union R.detailed,
      display: R.text
    }

The FHIR version appears to be returning a list of Codings, whereas the QICore version appears to be returning a list of Concepts. Also, the List casting with { E.ombCateogry } in Ethnicity but not in Race in the QICore version is interesting to me.

I think we need to loop in @brynrhodes in order to fully understand what the translator is doing for the SDE expression when using QICore and what the differences/expectations are there with the FHIR version.


W/r/t cql-execution, maybe something needs to change there in order to support nullish values getting returned from these SDE expressions. However, this issue only happens with the QICore version, not the base FHIR version, regardless of the presence of SDE extensions on the Patient bundle.

srankins commented 1 year ago

Thank you for the response @mgramigna. The SDE definitions mirror what is in the 2022 QI-Core eCQM Content repo for the SupplementalDataElement library (with the exception of the library name, which changed based on community consensus). Technically, the race and ethnicity extensions should have been provided in the Test Case JSON, since the measure is requesting that information, and those extensions are 'Must Support' in QI-Core, although the patient resource example in QI-Core does not provide them. Adding those extensions resolves the error for this particular instance.

Since this is the SDE library that will be used by the CMS measure developers, I think there should probably be a more graceful way of handling the error if a measure developer forgets to add the necessary data. We could update the SDE library to perform a null check before the union occurs, but that really only fixes this issue for the SDE library - seems like it could arise for other libraries as well. In addition to getting @brynrhodes thoughts, I would also be interested in seeing what @cmoesel thinks.

p9g commented 1 year ago

http://hl7.org/fhir/us/cqfmeasures/2023Jan/using-cql.html#concepts CQL Concepts are not currently used within measure development and SHALL NOT be used within FHIR-based eCQMs, except to the extent that individual codes will be implicitly converted to concepts for the purposes of comparison with the Concept-value elements in FHIR resources.

cmoesel commented 1 year ago

Based on @p9g's link above, it sounds to me like perhaps the SDE library using QICore will need to be modified anyway. The question is whether it's the Concepts that are causing the problem, or if it's something else (in which case refactoring out the use of Concept might not help anyway).

I have not personally run QICore-based CQL in the execution engine before, so I can't easily debug it -- but I can probably take a look at the ELM and see if anything jumps out at me as being off. I'm not sure I have time today, but I'll try to look soon. I'll also check in w/ @mgramigna to see if he has any more detail about his statement that "maybe something needs to change there in order to support nullish values getting returned from these SDE expressions."

srankins commented 1 year ago

@p9g , I agree, although I think the returns for Race and Ethnicity may not necessarily be a violation of this constraint. There's not much difference between using an attribute of a resource that returns a Concept and constructing the return of a definition as a Concept. But we may be splitting hairs here.

All that being said, I don't think it is accurate to stuff all the race codes for a patient into a Concept, since the OMB race codes convey different concepts. For example, 'American Indian or Alaska Native' !~ 'Black or African American', but, for a patient that was both 'American Indian or Alaska Native' and 'Black or African American', the SDE logic would combine them as part of the same Concept. The Ethnicity definition does something similar, but those OMB Ethnicity codes in the value set are mutually exclusive.

Handwaving over all of this, I think we would still have the issue of 'null union null' when there is no data present.

cmoesel commented 1 year ago

Rethinking this through again, and if the issue is null union null or { null } union null, then that means the expression is returning a Concept with either codes: null or codes: { null }. This breaks the logical model of what a Concept should be (as shown here), because a Concept is defined to have at least 1 Code. So... it's possible that perhaps the execution engine is assuming a Concept has at least one code and the error is related to it processing a Concept that breaks that assumption.

srankins commented 1 year ago

@cmoesel I only mentioned null union null because that is what the following would equate to when there is no data:

R.ombCategory union R.detailed

or

{ E.ombCategory } union E.detailed

However, in QI-Core ombCategory type is System.code, and detailed is List. It would have to be in order to use the output of that expression to create the equivalent type for construction of Concept (Concept.codes is type List). For reference, QI-Core 4.1.1 model info file is here.

It makes sense what you are saying though. It seems like this would happen because the author is literally constructing a Concept as opposed to returning an attribute typed Concept = null.

srankins commented 1 year ago

Interestingly enough, I updated the race and ethnicity definitions to return something other than a Concept, and the issue still occurs when there is no race and ethnicity data in the test JSON (i.e., still receive e.union is not a function). However, I then cast ombCategory to a list in the "SDE Race" definition - that is:

define "SDE Race": Patient.race R return Concept { codes: { R.ombCategory } union R.detailed, display: R.text }

I reran the execution (still no race and ethnicity data in the test JSON), and the error is gone.

cmoesel commented 1 year ago

Interesting... I guess that kind of makes sense though... By wrapping it with { }, you're forcing the null to become a list of null -- and now the engine knows how to union it.

In the US Core Race Extension, ombCategory is a list already (0..5) -- which is why the original CQL does not wrap it with { } (as opposed to US Core Ethnicity Extension, for which ombCategory is singular (0..1)). For that very reason, you don't really want the CQL to wrap the race extension's ombCategory in a list, because when there actually are codes there, you'll end up with an array inside an array. So don't keep it like that -- but thanks for doing it, as it helped to shed some more light on what's going on.

I think there is something for us to look at in the engine regarding this, but really, the best thing would be for the CQL to be more careful about constructing Concepts, because a Concept with null codes and display doesn't really make sense. You might want to consider taking an approach like this in order to ensure sensible Concepts:

define "SDE Ethnicity":
  Patient.ethnicity E
    return NullSafeCodesToConcept(
      { E.ombCategory } union E.detailed,
      E.text
    )

define "SDE Race":
  Patient.race R
    return NullSafeCodesToConcept(
      R.ombCategory union R.detailed,
      R.text
    )

define function NullSafeCodesToConcept(codes List<Code>, display String):
  if codes is null or Coalesce(codes) is null
  then null
  else Concept {
    codes: codes,
    display: display
  }
srankins commented 1 year ago

Thank you @cmoesel for the helpful info. I understand your comment about explicitly casting ombCategory to a list. I just found it interesting and wanted to share, in case it helped in troubleshooting. I am not the steward for this library. Again, this came about as an issue a user raised with MADiE. That said, SDE library is one of those common libraries that will be used by everyone, so it is important to get it right before everyone starts using it and then has to update their measures. For this reason, I'm really interested in getting @brynrhodes thoughts.

What I think I understand based on our back and forth today:

  1. The return for the Race and Ethnicity definitions should really not be a Concept. QMIG is explicit about constructing Concepts; an author is not supposed to do it. We can update these return statements pretty easily by removing the name of the Tuple, if indeed we still want them to be Tuples.
  2. When constructing types in CQL based on the physical layer of data, which may have null values, handle the nulls appropriately in the logic as opposed to pushing that responsibility over to the execution engine. At a minimum, understand the requirements and dependencies of the types being created and deal with them appropriately in the logic.
cmoesel commented 1 year ago

Yes, although I just realized that if this particular issue is with how the engine is performing those unions, then my suggested CQL above still does not solve this issue (because the CQL is still trying to do the union). Oops. It's been a long day.

p9g commented 1 year ago

It may be interesting to note how the SDE data gets packed into a MeasureReport http://hl7.org/fhir/us/davinci-deqm/2023Jan/MeasureReport-sde-example.html

The contained Patient has the SDE extensions, but other contained Observations also have the same codes, and extensions on the MeasureReport reference those Observations.

srankins commented 1 year ago

Does anyone know if @brynrhodes has been notified about this conversation? Do we need to send him a separate e-mail?

mgramigna commented 1 year ago

@srankins do you mind sending a separate email just to get it on the radar?

srankins commented 1 year ago

@srankins do you mind sending a separate email just to get it on the radar?

Done.

brynrhodes commented 1 year ago

Sorry for being late to the party :(. For the case of race specifically, where the ombCategory extension is multiple cardinality, the left argument to the union should be list-valued. Here's the relevant ELM:

<operand localId="34" locator="41:14-41:26" xsi:type="Query">
   <resultTypeSpecifier xsi:type="ListTypeSpecifier">
      <elementType name="t:Code" xsi:type="NamedTypeSpecifier"/>
   </resultTypeSpecifier>
   <source alias="$this">
      <expression xsi:type="Query">
         <source alias="$this">
            <expression path="extension" xsi:type="Property">
               <source name="R" xsi:type="AliasRef"/>
            </expression>
         </source>
         <where xsi:type="Equal">
            <operand name="ToString" libraryName="FHIRHelpers" xsi:type="FunctionRef">
               <operand path="url" scope="$this" xsi:type="Property"/>
            </operand>
            <operand valueType="t:String" value="ombCategory" xsi:type="Literal"/>
         </where>
         <return distinct="false">
            <expression path="value" xsi:type="Property">
               <source name="$this" xsi:type="AliasRef"/>
            </expression>
         </return>
      </expression>
   </source>
   <return distinct="false">
      <expression name="ToCode" libraryName="FHIRHelpers" xsi:type="FunctionRef">
         <operand name="$this" xsi:type="AliasRef"/>
      </expression>
   </return>
</operand>

In the case that there is no data in the extension element, that may return null, but it would be a "list-valued" null, which an engine should be able to union.

I've just committed a set of tests to the ecqm-content-qicore-2022 repository that demonstrates each case (ombcategory and detailed, only detailed, neither ombcategory nor detailed, and no race extension), and the results for the Java engine are:

Patient=Patient(id=example-with-race)
SDE Race=Concept {
    Code { code: 2106-3, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: White }
    Code { code: 1002-5, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: American Indian or Alaska Native }
    Code { code: 2028-9, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Asian }
    Code { code: 1586-7, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Shoshone }
    Code { code: 2036-2, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Filipino }
    Code { code: 1735-0, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Alaska Native }
}

Patient=Patient(id=example-without-category)
SDE Race=Concept {
    Code { code: 1586-7, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Shoshone }
    Code { code: 2036-2, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Filipino }
    Code { code: 1735-0, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Alaska Native }
}

Patient=Patient(id=example-without-codes)
SDE Race=Concept {
}

Patient=Patient(id=example-without-race)
SDE Race=null

Having said all that, I think there are definitely some specification issues that could use clarification:

1) The CQL Concept is intended to be the CQL representation matching FHIR's CodeableConcept. Although the type specification allows for concepts with no codes, or a null codes element, as well as a null display element, the syntax and (as @cmoesel points out) the ELM definition require at least one code to be specified. I think this is an oversight and that concepts without codes should be allowed, as CodeableConcepts without codes are certainly allowed, and so we need a way to represent that value. I think the CQL specification could use clarification on this point and so I've logged an issue here

  1. The reason the extensions are constructed as concepts in the SupplementalDataElements library was mostly one of convenience, since it is a structure that supports a list of Codes and an accompanying text description. We could just make it a Tuple and avoid the potential confusion there, but I'm not 100% sure that it's a violation of the notion of a CodeableConcept in FHIR, since they are all codes indicating the concept of ethnicity. I've always assumed that the designers of the USCore Race and Ethnicity extensions intentionally modeled them the same way as CodeableConcepts so that they could be easily represented with that structure. I could be wrong about that, it would be worth a discussion with terminology and/or the USCore team on that point.

  2. As @p9g points out, the QM IG does prohibit the use of Concepts except as they arise in the context of comparisons with Concept-valued elements in FHIR resources, which I would argue that this is such a case. I'd be happy to add a clarification to that conformance requirement to make that clear though if you wanted to submit a ballot comment on the QM IG currently in ballot for that.

  3. And having said all that, I wouldn't be opposed to changing the SupplementalDataElements library to construct a Tuple rather than a Concept to potentially avoid the semantic mixing going on there, however, I don't think that would address the technical issue here, since we'd still have to union potentially null list values to get all the codes we need.

Thoughts?

srankins commented 1 year ago

My only thought around Concept - Based on a Concept's definition, my understanding is that a Concept is supposed to contain equivalent codes across different code systems or even within the same code system. Are we conflating the Concept for Race if we have non-equivalent race codes?

Given the comment from @brynrhodes about updating CQL to allow for concepts with no codes, @cmoesel, would cql-execution be updated after this change takes place, or would the update be made simultaneously?

cmoesel commented 1 year ago

In the case that there is no data in the extension element, that may return null, but it would be a "list-valued" null, which an engine should be able to union.

I agree. I was just remarking that in the case of the JavaScript engine, that only comes across as JavaScript null, with no additional typing to indicate it is "list-flavored". That's a limitation in the engine. Still, I think we should be able to handle it better than we are.

I think this is an oversight and that concepts without codes should be allowed, as CodeableConcepts without codes are certainly allowed, and so we need a way to represent that value.

Agreed. Although I think a Concept with no codes has very limited value in CQL, since it's a construct used in FHIR, we at least ought to allow for it.

I'm not 100% sure that it's a violation of the notion of a CodeableConcept in FHIR, since they are all codes indicating the concept of ethnicity.

I agree with @srankins on this one; the individual Codings should represent the same concept. The only variation discussed in the FHIR spec is either variations in the system used or variations in granularity. In this case, the concepts in the coding do not represent the concept meaning "ethnicity" (e.g., the question), but rather, they represent specific (different) ethnicities (e.g., the answers).

But... the CQL specification makes this even more clear in its definition of ConceptDef: "All codes within a given concept must be synonyms." Although that's for ConceptDef (not Concept) I think it can be assumed to apply to Concept since ConceptDef's only purpose is to create Concepts.

I've always assumed that the designers of the USCore Race and Ethnicity extensions intentionally modeled them the same way as CodeableConcepts so that they could be easily represented with that structure.

Perhaps, but I actually took it the exact opposite way. If they intended it to be easily represented in a CodeableConcept, wouldn't they have made the detailed sub-extension a CodeableConcept? That surely would have been easier than separate sub-extensions for the codings and text. To me, the separate sub-extensions indicate intentional avoidance of using CodeableConcept.

(All that said, it begs the question of how you would represent multi-valued ethnicity in an Observation? A CodeableConcept certainly would be easiest, but still a questionable use of CodeableConcept.)

I wouldn't be opposed to changing the SupplementalDataElements library to construct a Tuple rather than a Concept to potentially avoid the semantic mixing going on there

Based on my understanding of the intent of CodeableConcept, and CQL's statement that a concept's codes must be synonyms, it seems technically inappropriate to use a Concept here.

I don't think that would address the technical issue here, since we'd still have to union potentially null list values to get all the codes we need.

Agreed. That something we need to fix in cql-execution anyway.

would cql-execution be updated after this change takes place, or would the update be made simultaneously?

There's no hard dependency here, so I think those updates can happen independently.

cmoesel commented 1 year ago

@brynrhodes - I think I've tracked down the issue in cql-execution. It has to do with how we handle queries for which the source is null. When the query source is null, we can't tell if the source is supposed to be a list or an object -- which affects how we return the query results.

This leads me to a general question, however. As far as I can tell, the CQL spec doesn't specify what should happen when a query's source is null - and it seems there are a few different ways that could go. What do you think?

// Does the query below return null, an empty list, or a one-item list containing a Concept w/ empty or null codings?
define NullListQuery:
  (null as List<FHIR.CodeableConcept>) L
    return Concept {
      codes: L.coding C return FHIRHelpers.ToCode(C)
    }

// Does the query below return null or a Concept w/ empty or null codings?
define NullObjectQuery:
  (null as FHIR.CodeableConcept) O
    return Concept {
      codes: O.coding C return FHIRHelpers.ToCode(C)
    }
cmoesel commented 1 year ago

Given that CQL has a tendency to propagate nulls in general, I'm guessing (and hoping) that both those queries return null. 🤞

srankins commented 1 year ago

Checking on status of this ticket. It has been a little while since we have been able to discuss. It looks like @cmoesel had a question for @brynrhodes or the group based on identification of the issue within cql-execution, which I'm not sure has been answered. Based on previous conversations at eCQM Standards Coordination meeting, there were concerns about some of the content in the SDE library around concept structures used for the ethnicity and race definitions. It sounded like updates would be made to the SDE library, but there may still be lingering questions or conversations around library updates.

mgramigna commented 1 year ago

Hi @srankins

@brynrhodes @cmoesel and I got together at the connectathon to discuss this issue more. This resulted in https://jira.hl7.org/browse/FHIR-40225, which will add a clarification to the CQL about how to handle null-sourced queries (details in the ticket)

cql-execution will be updated to ensure that null-sourced queries return null, which will fix the doUnion issue from the original post on this issue. This update will then immediately be pulled into fqm-execution, and we will release a new patch version.

srankins commented 1 year ago

@mgramigna, thank you for the update. Also, it was good to meet @laclark22 and you at the Connectathon - finally can put some faces to some names. Regarding the solution for SDE, do you know if there is a plan to update SDE library?

cmoesel commented 1 year ago

@srankins -- It looks like the SDE in ecqm-content-qicore-2022 was updated very recently. The relevant definitions are now returning Tuple instead of Concept. See: https://github.com/cqframework/ecqm-content-qicore-2022/blob/master/input/cql/SupplementalDataElements.cql

srankins commented 1 year ago

Thanks @cmoesel. I think this is a question that will have to be discussed at one of the measure developer meetings, since it will have an impact on the work they are doing. @brynrhodes and I are at the WGM this week, so I will ask him what the plan is.

mgramigna commented 1 year ago

Fixed in v1.0.4