-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
e.union is not a function #172
e.union is not a function #172
Comments
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 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. |
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. |
Yeah, makes total sense. I'll give it a good look tomorrow and we can identify next steps. Thanks! |
This may not matter, but the measure bundle is missing .id for the Bundle and the Measure. |
@p9g That shouldn't affect calculation for the fqm-execution since it doesn't rely on resource IDs |
@srankins sorry for the late response. I did some digging, and have confirmed my above suspicion. TL;DR: The translator + 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 I think we need to loop in @brynrhodes in order to fully understand what the translator is doing for the SDE expression when W/r/t |
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. |
http://hl7.org/fhir/us/cqfmeasures/2023Jan/using-cql.html#concepts |
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 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." |
@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. |
Rethinking this through again, and if the issue is |
@cmoesel I only mentioned null union null because that is what the following would equate to when there is no data:
or
However, in QI-Core ombCategory type is System.code, and detailed is List<Sytem.code>. 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<System.code>). 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. |
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:
I reran the execution (still no race and ethnicity data in the test JSON), and the error is gone. |
Interesting... I guess that kind of makes sense though... By wrapping it with In the US Core Race Extension, ombCategory is a list already (0..5) -- which is why the original CQL does not wrap it with 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
|
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:
|
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. |
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. |
Does anyone know if @brynrhodes has been notified about this conversation? Do we need to send him a separate e-mail? |
@srankins do you mind sending a separate email just to get it on the radar? |
Done. |
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:
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:
Having said all that, I think there are definitely some specification issues that could use clarification:
Thoughts? |
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? |
I agree. I was just remarking that in the case of the JavaScript engine, that only comes across as JavaScript
Agreed. Although I think a
I agree with @srankins on this one; the individual 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
Perhaps, but I actually took it the exact opposite way. If they intended it to be easily represented in a (All that said, it begs the question of how you would represent multi-valued ethnicity in an
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.
Agreed. That something we need to fix in cql-execution anyway.
There's no hard dependency here, so I think those updates can happen independently. |
@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?
|
Given that CQL has a tendency to propagate nulls in general, I'm guessing (and hoping) that both those queries return null. 🤞 |
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. |
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)
|
@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? |
@srankins -- It looks like the SDE in |
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. |
Fixed in v1.0.4 |
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?
The text was updated successfully, but these errors were encountered: