Coaelse with an interval is not working properly if star/end is not specified
Closed this issue · 14 comments
CQL execution engine is setting null to minimum or maximum value for an interval's lower or upper boundary depending on where the null is entered. This behavior impacts the evaluation of Coaelse logic and creates false positives or negatives depending on the use case.
In QDM and FHIR measures the following construct with Coaelse from a list of intervals is frequently used :
define function MedicationOrderPeriod(Order "Medication, Order"): if Global.HasEnd(Order.relevantPeriod) then Interval[Coalesce(start of Order.relevantPeriod, Order.authorDatetime), end of Order.relevantPeriod]
Other samples can be found here:
https://raw.githubusercontent.com/cqframework/CQL-Formatting-and-Usage-Wiki/master/Source/Libraries/MATGlobalCommonFunctions-7.0.000.md
https://raw.githubusercontent.com/cqframework/CQL-Formatting-and-Usage-Wiki/master/Source/Libraries/CumulativeMedicationDuration-1.0.000.md
All intervals that come from Bonnie (or cql-exec-fhir) are closed/closed. Thus, Order.relevantPeriod.start from the example above will become 1-1-1T00:00 and will be returned from Coalse as a first not null value.
Where the interval is constructed in CQL expressions, the CQL is incorrect there, this is a known issue with the CMD library (will be addressed as part of an update presented on the Cooking with CQL this Thursday). If there are other cases in MATGlobalCommonFunctions, can you point to them specifically? I'm happy to address those as well, but I don't think this is an engine issue, the engine is doing what we told it to.
@brynrhodes , are you saying that a coalesce should not be used in construction of an interval? I don't see that restriction in the standard, and it seems to be used in various places throughout MATGlobalCommonFunctions v 7.0.000, the CMD library and the FHIR version of MATGlobalCommonFunctions, so I do not believe that is what you are saying. But then my question would be why the CQL is then wrong. As we have reviewed, what seems to be happening is the following;
null provided by user for lower bound of interval in GUI
null passes over to cql-execution and is translated to minimum value
cql execution then occurs, but, because the null value no longer exists, the coalesce picks up the minimum value
If you want to discuss via zoom, let us know, or we can wait for Friday's meeting, if helpful.
@brynrhodes , just an FYI, I was able to get this same scenario to recreate by use of the following:
define "Prostate Cancer Period"
singleton from ( ["Diagnosis": "Prostate Cancer Diagnosis"] Dx
return Interval[Coalesce(start of Dx.prevalencePeriod, Dx.authorDatetime), end of Dx.prevalencePeriod] )
define "Initial Population":
"Prostate Cancer Period" during "Measurement Period"
data:
Measurement Period: 2012
relPeriod start: null
relPeriod end: 12-13-2012
authorDatetime: 2-1-2012
So it does appear that this situation will happen anytime Coalesce is used with a null value for the lower or upper boundary of an interval. Looking back through the Global library version 7.0.000, this impacts the following functions:
- Hospitalization
- HospitalizationWithObservationAndOutpatientSurgeryService -
- HospitalizationWithObservation
This impacts the following in the CMD library:
- MedicationOrderPeriod
- MedicationDispensedPeriod
Look forward to discussions.
Hi @JSRankins. The issue is that a closed interval boundary set to null represents the lowest/highest value for the point type. As a result, the spec says this about the start of operator on intervals (link):
If the low boundary of the interval is closed and the low value of the interval is not null, this operator returns the low value of the interval. Otherwise, the result is the minimum value of the point type of the interval.
So... assuming that the interval is actually using a closed low boundary, the engine is working exactly as it should, because the start of changes the null value to an actual value (the minimum date value).
But... since the incoming argument is actually a FHIR.Period, it's not as clear what that low boundary is (open or closed). Prior to CQL Translator PR #557 (almost exactly one year ago), ToInterval was defined like this:
define function ToInterval(period FHIR.Period):
if period is null then
null
else
Interval[period."start".value, period."end".value]
which meant that all converted intervals would be closed boundaries (and thus run into the issue above). That PR, however, fixes it so it aligns better w/ the FHIR spec, which says "If the start element is missing, the start of the period is not known. If the end element is missing, it means that the period is ongoing." So now it is defined as:
define function ToInterval(period FHIR.Period):
if period is null then
null
else
if period."start" is null then
Interval(period."start".value, period."end".value]
else
Interval[period."start".value, period."end".value]
The question is, which ToInterval is being used in your case? The old one or the new one?
I'd also note, however, that in your example, if you are using the latest FHIRHelpers, you could still run into problems because you're forcing a closed low boundary on the interval you're creating -- but it's using intervals w/ unknown boundary status as its input -- so it's possible you might change an open null (unknown) into a closed null (minimum datetime), which is also not great.
Hi @cmoesel!! Hope you are doing well. This is Stan. The links that @serhii-ilin shared are referring to QDM. Not FHIR. Though I assume this problem is in FHIR too like you are saying; I have not been able to look yet. Your explanation confirms what we were thinking is happening. However, the 'easy fix' at this point is to rewrite the impacted CQL in the libraries where it exists. Unfortunately, that means a good chunk of CQL in the MAT Global and CMD libraries, which was already vetted and approved by the community. I will leave that up to @brynrhodes. As it is, the Translator does not provide any errors when Coalesce is used on a lower or upper bound; I'm not sure if that should be the long-term resolution, since there are cases where the lower or upper bound is provided, or for the execution engine to throw an error when encountering this issue. It just seems that we would want a fix to prevent something like this situation occurring in the future dependent upon what happens with QDM. Thoughts?
Hi Stan! I totally missed that this was QDM-based, but I see that now. Doh. Sorry! I guess I don't know how QDM represents its periods as intervals -- but the bottom line is that if the data distinguishes between an "unknown" boundary and an "infinite" boundary, then that needs to be reflected as an open or closed boundary (respectively) in the CQL Interval. If the data doesn't distinguish between those concepts "unknown" vs "infinite", then I guess a convention needs to be defined to determine how to resolve that issue. I'll leave it to you, @brynrhodes, and @serhii-ilin to determine next steps, but I think the engine is working as-designed and according to spec -- so I'm not sure that having the engine throw an error would be appropriate in any case.
Thanks @cmoesel. I think the issue is that the user wants an infinite boundary but the user wants the ability to set the boundary value in the CQL based on the presence or absence of data for specific boundaries. Since the null gets changed to minimum value before the Coalesce is executed, that can't happen using the current solution. I assume a test could be made for whether the lower or upper bound is min or max instead of using Coalesce, but then that means that Coalesce can't really be used for lower or upper boundary checks. Maybe that is the intent, but I am not finding that clarity defined in CQL.
@JSRankins -- I wonder if you can use the .low property on the interval instead? In other words, instead of start of Dx.prevalencePeriod, try Dx.prevalencePeriod.low. I don't think the engine will convert that to min value (although I'm not 100% sure of the behavior). It's probably worth a try anyway.
@cmoesel and @brynrhodes - Looks like we have a similar issue with the CMD library. It involves the use of collapse when the quantity to use is not specified in the CQL - e.g., collapse Intervals as opposed to collapse intervals per day. Without the quantity being specified in the CQL, the CQL errors on runtime once reaching this portion of the "CumulativeDuration" function. Let me know if you want me to open a separate ticket for this issue, so this one can be closed.
Of course, once this 'per quantity' is entered, everything is fine until the "CumulativeMedicationDuration" function is reached. An execution error occurs when hitting the 'is' operation in that function. Bonnie is currently on 2.2.0, so I thought 'is' was working. Did something get updated after 2.2.0?
@cmoesel or @brynrhodes, any thoughts on this latest issue? Would it help discuss on a call?
Hi @JSRankins -- it would be helpful to have some more specifics regarding the errors you're seeing w/ collapse. Is there an error message? A stacktrace? This does sound like a separate issue to me -- so it's probably worth filing separately.
Regarding the error with is, again, it would be helpful to have some more details (error message, stack trace, etc).
Sounds great @cmoesel I will open up two tickets - one for the collapse without units and the other for the is operator. The stacktraces will be provided in those tickets along with the respective CQL.
Yeah. I think this could probably be closed. @serhii-ilin, do you agree? This dealt with another issue that was resolved with an update to the CumulativeMedicationDuration library. #255 has been addressed through a release today. #256 will be addressed in Bonnie.