Changed use of exec to execute for expressions

[elsaperelli]

Pull requests into cql-execution require the following.
Submitter and reviewer should ✔ when done.
For items that are not-applicable, mark "N/A" and ✔.

CDS Connect and Bonnie are the main users of this repository.
It is strongly recommended to include a person from each of those projects as a reviewer.

Summary

@hossenlopp and I worked on this pull request together and found that the AnyInValueSet class that extends from Expression contained code in its exec function that called exec() on this.codes. Since this.exec was being called instead of this.execute, the localId of this.codes and its result were not being set on the root context, thus causing situations in which this localId and result were missing. By making this change, this.execute now gets properly called. I wasn't sure about unit testing for this case, so I created a new one that checks the three inputs to codes in AnyInValueSet (list of strings, codes, or concepts) (defined here). Additionally, there are now checks that ensure the localId of the codes expression is present on the localId_context.

After finding this, we wanted to check the rest of the code for other instances where .exec was being used when it should be .execute(). There were only a few places where this occurred: First, Last, and Slice in list.ts and AggregateClause in query.ts. I did not add any additional unit tests for these changes, but I did add checks for the existence of the localId of the expression on the root context.

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

Merging #314 (6900c67) into master (cc3ff79) will not change coverage.
The diff coverage is 85.71%.

❗ Current head 6900c67 differs from pull request most recent head 8171aa6. Consider uploading reports for the commit 8171aa6 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master     #314   +/-   ##
=======================================
  Coverage   86.15%   86.15%           
=======================================
  Files          52       52           
  Lines        4508     4508           
  Branches     1270     1270           
=======================================
  Hits         3884     3884           
  Misses        324      324           
  Partials      300      300           

Files Changed	Coverage Δ
src/elm/query.ts	90.78% <0.00%> (ø)
src/elm/clinical.ts	82.64% <100.00%> (ø)
src/elm/list.ts	96.15% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[cmoesel]

Good finds!

I noticed one spot in AggregateClause where the exec was not changed to execute. Is that intentional?

I also made one comment about the tests and the potential brittleness of using the local ids directly. Let me know what you think.

[cmoesel]

I like the idea of these tests. My only concern is that the local ids in the ELM might change if we upgrade the CQL Translator or if we modify the CQL test cases that are being translated. For that reason, it might be better to actually extract the localId from the built expression. E.g.

should(this.ctx.localId_context[this.anyInListOfConcepts.codes.localId]).not.be.undefined();

Of course, this would be fairly annoying to do at scale. But if all the numbers get offset by one in the future, then that would also be fairly annoying to fix at scale. ;-). Referencing the localId from the expression also provides better confidence that you're checking the right one.

[elsaperelli]

Good call! This will be easy to fix- thank you!

[cmoesel]

@elsaperelli - I apologize for not calling it out explicitly, but quite a few local ids were also added in the list-test.ts and query-test.ts files as well... This is why I said it would be pretty annoying to fix at scale -- because, dang, it will be.

The tests work now -- it's more about preventing unintended surprises in the future. So if we want to hold off on updating the rest of those tests so we can get this PR out the door, I totally understand.

[elsaperelli]

Good finds!

I noticed one spot in AggregateClause where the exec was not changed to execute. Is that intentional?

I also made one comment about the tests and the potential brittleness of using the local ids directly. Let me know what you think.

Interesting point about that line in AggregateClause! Truthfully, I am not totally sure. I think I originally missed it/ignored it because it takes childContext and not ctx, but perhaps that doesn't matter. What do you think? Should this be changed to execute as well?

[cmoesel]

Interesting point about that line in AggregateClause! Truthfully, I am not totally sure. I think I originally missed it/ignored it because it takes childContext and not ctx, but perhaps that doesn't matter. What do you think? Should this be changed to execute as well?

Honestly, I don't know. I suspect it wouldn't hurt, but I also understand if you don't want to make any last-minute changes to something you've already tested. If you want to try changing it to execute and confirm it doesn't cause issues, then great! But if you'd rather hold off and just get this PR in as-is, that's totally fine too.

[birick1]

I've been curious about AggregateClause too. Also looking to see if I can find any issues with a change.

[cmoesel]

OK. So I had some time to look at the code. Here's the deal with executing the expression in the AggregateClause...

The expression gets executed multiple times (once per item in the list being aggregated). Even though it is being executed w/ a child context, execute invokes ctx.rootContext().setLocalIdWithResult(this.localId, execValue). So the local id gets recorded on the root context (which I think is the PatientContext in this case) -- so there's just one master collection of the localIds and their values.

So setLocalIdWithResult gets invoked multiple times with the same localId, but different values. When it sets it on subsequent invocations, it looks like it only sets it if the old value was falsy (null, undefined, false, 0, or 0-length). So at the end of execution, that local id that was set multiple times in a loop will always be the first truthy value it was set with (if applicable). This is kind of weird, but I'm guessing it's because of how Bonnie (and now FQM) did coverage.

Note that other clauses in a query (like where, with, such that, return, sort, etc) have the same behavior -- they're iterated with each item in the source list too. where, with, such that, and return use execute, so they set local id in the root context's map using the same semantics described above (first truthy value wins). sort, however, uses only exec and even has a comment saying that this is intentional -- but the By expresion (in sort by) uses execute.

I think that the AggregateClause is more like return than it is like sort, so if I had to make a call, I'd say that the AggregateClause should use execute -- even if the semantics of iterative expressions w/ local ids is wonky. It's at least consistently wonky with other similar components of queries.

[cmoesel]

This looks great! Thanks for finding all the spots we missed over the years!

As discussed on Slack, we'll save the annoying task of "de-brittling" some of those tests for a future PR.