Cache function ref #326
Cache function ref #326
Conversation
The npx codecov script is deprecated, so use the GitHub action instead.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #326 +/- ##
==========================================
+ Coverage 86.15% 87.06% +0.91%
==========================================
Files 52 52
Lines 4508 4524 +16
Branches 1270 1275 +5
==========================================
+ Hits 3884 3939 +55
- Misses 324 377 +53
+ Partials 300 208 -92 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
My only concern with this approach was caching the resolved function for reuse across iterations, but I can't think of a use case where different iterations of the same FunctionRef would result in different function resolutions. I tried to force it with CQL like this:
define function ValueAsString(value FHIR.CodeableConcept):
Coalesce(value.text.value, value.coding[0].display.value, value.coding[0].code.value, 'unknown')
define function ValueAsString(value FHIR.Quantity):
ToString(value.value.value) + ' ' + Coalesce(value.unit.value, value.code.value)
define PatienObservationsAsStrings:
[Observation] O return ValueAsString(O.value as Choice<FHIR.CodeableConcept, FHIR.Quantity>)
but the CQL-to-ELM compiler doesn't allow it because the function invocation is ambiguous. You have to do something like this, which results in two independency FunctionRefs:
define PatienObservationsAsStrings:
[Observation] O return
case
when O.value is FHIR.CodeableConcept then ValueAsString(O.value as FHIR.CodeableConcept)
when O.value is FHIR.Quantity then ValueAsString(O.value as FHIR.Quantity)
else 'unsupported'
end
All that to say, I think this is safe and it appears to work. Still, I'd like for the FLAME team to have a chance to review and possibly regress it against their measures. @hossenlopp - do you agree that your team should review and regress this?
|
I'll also note that the npm audit is unresolvable because the vulnerable module has not yet released a patch. |
This is primarily a performance enhancement. When function use is defined within a Query, every iteration of elements in the where clause caused a re-evaluation of finding the correct FunctionDef to use. When there are a large number of items to go through this becomes a very expensive process, mainly due to the type comparisons that are used to perform the FunctionDef lookups.
This PR implements caching of the FunctionDef look ups with a FunctionRef the first time it is executed.
Note that this PR also contains updates to 3 node modules that were not passing audit tests
Pull requests into cql-execution require the following.
Submitter and reviewer should ✔ when done.
For items that are not-applicable, mark "N/A" and ✔.
This is primarily a performance enhancement. When function use is defined within a Query, every iteration of elements in the where clause caused a re-evaluation of finding the correct FunctionDef to use. When there are a large number of items to go through this becomes a very expensive process, mainly due to the type comparisons that are used to perform the FunctionDef lookups.
This PR implements caching of the FunctionDef look ups with a FunctionRef the first time it is executed.
Note that this PR also contains updates to 3 node modules that were not passing audit tests
Pull requests into cql-execution require the following.
Submitter and reviewer should ✔ when done.
For items that are not-applicable, mark "N/A" and ✔.
Submitter:
npm run test:plusto run tests, lint, and prettier)cql4browsers.jsbuilt withnpm run build:browserifyif source changed.Reviewer:
Name: