Implement Retrieve:codeComparator

[dehall]

TL;DR:

This PR implements Retrieve.codeComparator, so retrieves now honor in, =, and ~ semantics for ValueSets, Codes, Concepts, and code lists, including multi-valued code properties. Exact equality is now supported and compares code displays, while equivalence continues to match code/system only. CodeSystem retrieves are intentionally unsupported but wired up so that if the appropriate terminology lookups are ever implemented, they will work here.

---

This PR implements proper support for the codeComparator property of a Retrieve (implements #293). After a lot of thinking, testing, and discussion about this, it turns out this PR doesn't change that much about how Retrieves are handled but it does tune the implementation to be a little more aligned to the spec, and fixes a few cases that were not on the happy path.

Spec / Background

For context, there are 3 key parts to a Retrieve that are relevant to this discussion. 1, the codeProperty, 2, the codeComparator, and 3, the codes (or terminology). For example, in [Condition: bodySite in "myValueSet"] -- codeProperty is bodySite, codeComparator is in, and codes is myValueSet. If the codes is specified but the codeProperty and codeComparator are not, those fields are populated in translation based on the model. (there's a default codeProperty for every resource type, and a default operator for whichever type codes is). So codeComparator is always present, and currently allows 3 values (~, =, and in).

Note from this point on I'll refer to the codeProperty and the value of that field on a given resource as the "Left-Hand Side" or LHS, and I'll refer to the codes or terminology that it's being compared to as the "Right-Hand Side" or RHS. So eg, bodySite in myValueSet, bodySite is the LHS and myValueSet is the RHS. Otherwise referring to "codes" gets too confusing.

Implementation

The core of the comparison logic is performed in Retrieve.recordMatchesCodeOrVS (src/elm/external.ts). Previously, this would only look at the type of the RHS to appropriately call hasMatch on either a single ValueSet object or on the elements of a code list. Now, this branches out into 3 separate functions for each codeComparator type, with further branches based on the type of the RHS to call the appropriate function.

Now, the happy path of this is pretty easy to see what should happen: [Condition: code in "myValueSet"] or [Condition: code = "mySingleCode"]. But it turns out that the operators in a Retrieve operate differently than they do in a standalone definition, and are much more permissive. Combinations that would be generally be illegal, eg "mySingleCode" = "myCodeSystem", are allowed in a Retrieve, eg [Condition: code = "myCodeSystem"]. We discussed this a little bit on Zulip: #cql > Retrieve.codeComparator implementation details
The takeaway from that is that it is intentional, it's meant to be flexible but also consistent for the user, but the execution engine does have to concern itself with the different combinations.

However, while the ELM translator permits every combination of operators and operands in a Retrieve, even combinations that are not allowed standalone, for valid combinations it does try to be smart about the types, and will list-promote or expand value sets in certain cases. For example:
[Condition: code = "myValueSet"] --> the resulting Retrieve.codes is an ExpandValueSet on a ValueSetRef and gets passed into recordMatchesCodeOrVS as a list of Codes
[Condition: code in "myValueSet"] --> the resulting Retrieve.codes is a plain ValueSetRef and gets passed into recordMatchesCodeOrVS as a ValueSet object
So my takeaway here is, we should generally consider a ValueSet and Code[] to be equivalent, if possible.

And beyond that, it actually turns out that because of list promotion, even if in the CQL the RHS is a single Code, it will be represented in the resulting Retrieve.codes as a ToList wrapping that code. So even if the CQL says code = myCode, it's really code = { myCode }.
This means that there's no way to distinguish from the ELM whether the CQL was originally written as a single code or as a list/Concept/ValueSet that contained only a single code. So that greatly reduces the number of combinations we have to consider, but it also means that some of the trivial combinations get folded up into less obvious combinations and we have to make implementation decisions.

Start with the = operator for equality. So now implementing this I see a few options:

1. Use regular list equality, LHS = RHS. { code1 } = { code1 } is true, but { code1 } = { code1, code2 } is false.
2. Interpret the intent of the operator as "some exact match exists between the LHS and RHS". { code1 } = { code1, code2 } is now true.
3. Different logic depending on the size of RHS. Assume that a list with a single item was list-promoted, anything larger was originally a list.

Let's consider a couple examples to help decide:

• [Condition: code = mySingleCode]
  • I imagine this is probably the most common example
  • Condition.code is 0..1 so this would likely work "as expected" no matter how we implement it
• [Condition: bodySite = mySingleCode] - reminder Condition.bodySite is 0..*.
  • If a given condition has 2 bodySite codes, this could become a comparison { code1, code2 } = { code1 } . Should this evaluate to true? I think so.
  • Option (1) means "exactly this list" which may be uncommon in a retrieve. (1) would return false in this example.
• [Condition: bodySite = myConcept]
  • Again bodySite is 0..*, and a concept may have multiple codes. List equality as (1) means "exactly this list" which could be a valid use case, but I expect most of the time that's not what's expected.
• [Condition: bodySite = myCodeList]
  • What if someone wanted to search for a Condition with exactly a specific list of codes, ie, not just a single code but multiple? Regular list equality as in (1) would allow this possibility, but (2) does not.

Approach (3) might produce a better result in some of these cases, but would also result in inconsistent and potentially unexpected behavior in others, so my preference is not to go that route.
So that leaves (2). More permissive than (1), there's no example that should return false in (2) but true in (1).

Final Implementation Approach

So after all that, I implemented the operators with the interpretation that ~ and = mean "some element from the LHS matches some element from the RHS", and not "the entirety of the LHS matches the entirety of the RHS". in for CodeSystems and in for ValueSets are both defined to have equivalence semantics, so ~ and in are essentially the same, and both implement essentially the same logic as previously existed. So the only thing that is really "new" here is support for the = operator, as well as more specific Error messages when calling the few illegal combinations.

Supporting Changes

• In order for any comparison using = to work, the code display needs to be populated, but previously there were some paths that copy code objects without copying the display. Those were fixed here, and various test fixtures and test cases were updated as well. (Possibly more than needed but only for consistency within a single file, eg, the valuesets fixture)
• The logic that looks up the codeProperty had been assuming the field was a single code, but this is not necessarily true and there are many fields that are a list of codes, eg Condition.bodySite which is a 0..* CodeableConcept. Ideally there would be a data-model-aware generic getter, but for now that doesn't exist. I wasn't sure whether it was better to update getCode to possibly return a list, so I added a new function getCodeOrCodes which always returns a list even if getting a single code.
• Added a CodeSystem.hasMatch function that just throws an error for now. The InCodeSystem operation is not supported yet (Support InCodeSystem/AnyInCodeSystem #289) and too large to implement here, and it felt cleaner to add that "not yet implemented" error in a CodeSystem member function rather than the function handling the in operator. However, it turns out the code execution flow doesn't make it this far, since earlier on the code tries to evaluate the CodeSystem as if it were a ValueSet and that lookup fails. I didn't modify that earlier step since there's no real way to make it work without a full implementation, so the impact is just that the error message might not be the most ideal for the situation.

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:

• This pull request describes why these changes were made
• Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
• Tests are included and test edge cases
• Tests have been run locally and pass
• Code coverage has not gone down and all code touched or added is covered.
• Code passes lint and prettier (hint: use npm run test:plus to run tests, lint, and prettier)
• All dependent libraries are appropriately updated or have a corresponding PR related to this change
• cql4browsers.js built with npm run build:browserify if source changed.

Reviewer:

Name:

• Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
• The tests appropriately test the new code, including edge cases
• You have tried to break the code

[codecov-commenter]

Codecov Report

❌ Patch coverage is 68.29268% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.24%. Comparing base (5c28fd1) to head (9468349).

Files with missing lines	Patch %	Lines
src/elm/external.ts	64.70%	8 Missing and 4 partials ⚠️
src/datatypes/clinical.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
- Coverage   88.41%   88.24%   -0.17%     
==========================================
  Files          54       54              
  Lines        4832     4867      +35     
  Branches     1359     1372      +13     
==========================================
+ Hits         4272     4295      +23     
- Misses        368      377       +9     
- Partials      192      195       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cmoesel]

Thanks for the helpful description and context for this PR. You've thought through this well and I think the approach makes sense. I've made a few suggestions for your consideration. My biggest concern is that I don't think we need to break backward compatibility for this, so I'd like to adjust where necessary to ensure existing solutions (and ELMs) continue to work.

[cmoesel]

This is looking good and most of it makes sense to me. The only thing I struggled with was the actual comment about the retrieve implementation. See my individual comment for more.

[cmoesel]

This looks good to me. I regressed it against a bunch of old CDS Connect artifacts and all the tests pass. Of course I'd like confirmation that it passes fqm-execution regression too before merging.

[hossenlopp]

Tested in fqm-execution against a large set of CMS Measures. No changes to calculation as expected.