WIP of HighBoundary and LowBoundary

[dehall]

This PR is a Work-In-Progress of implementing the HighBoundary and LowBoundary operators. (closes #159)

Turns out this depends on CQL Decimal specifics, so I didn't want to implement this before the upcoming "BigDecimal" work. Putting this up as a WIP/Draft mostly so that we don't forget about it if the BigDecimal effort takes a long time. The Date/Time/DateTime versions should be functional but there is likely some cleanup and de-duplication possible.

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 57.14286% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.25%. Comparing base (5c28fd1) to head (7a1fdeb).

Files with missing lines	Patch %	Lines
src/datatypes/datetime.ts	68.75%	14 Missing and 1 partial ⚠️
src/elm/arithmetic.ts	20.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
- Coverage   88.41%   88.25%   -0.16%     
==========================================
  Files          54       54              
  Lines        4832     4895      +63     
  Branches     1359     1376      +17     
==========================================
+ Hits         4272     4320      +48     
- Misses        368      379      +11     
- Partials      192      196       +4     

☔ 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]

Well, this is kind of embarrassing, but... I just found an implement_missing_operators branch that I totally forgot about. It has a complete implementation of LowBoundary and HighBoundary already. I just rebased it. Should we just go with that?

[dehall]

Well, this is kind of embarrassing, but... I just found an implement_missing_operators branch that I totally forgot about. It has a complete implementation of LowBoundary and HighBoundary already. I just rebased it. Should we just go with that?

Your branch looks good for date/time but the numeric piece might need to be updated. From the CQL 1.5.3 spec: https://cql.hl7.org/09-b-cqlreference.html#highboundary

HighBoundary(1.587, 8) // 1.58799999

but that doesn't fully make sense to me since 1.58799999 doesn't round to 1.587.

I think others agreed because if you look at latest build spec: https://build.fhir.org/ig/HL7/cql/09-b-cqlreference.html#highboundary

HighBoundary(1.587) // 1.58750000

which I agree with, if you think of it as the high boundary of an interval with an open endpoint.

Plus since I'm working on Decimals at the moment there would need to be rebasing anyway, so let's hold off for now and revisit that after the new Decimals are in.

[cmoesel]

Ah, right. I totally forgot that LowBoundary and HighBoundary were clarified and examples changed. That all came out of this long Zulip thread: #fhirpath > Boundary functions @ 💬 (which is about FHIRPath implementations, but it applies to CQL as well).

I'll admit that HighBoundary(1.587) // 1.58750000 confused me since 1.58750000 rounds to 1.588, but Grahame's comment on that thread provides the insight/justification I needed. In a discussion about whether the high boundary of 1 is 1.49999999 or 1.5, he said:

the high boundary is - or should be .5, not .49999

the grounds for this is that the boundary has infinite precision in theory but not practice

so if you state the high boundary as 1.49999, is 1.499996 in the boundary of 1

it has to be, so that's why the boundary needs to be stated as 1.5, not 1.4999999, even though that means in principle that 1.50000000000000000 would be rounded up to 2, not down to 1

It still feels a little off to me (since, for example, 1.499999996 doesn't exist in a system that only supports 8 decimals of precision), but I at least understand the justification now -- and honestly it's not likely to make a tangible difference in the real world.

As for what we do with this PR and my branch, I'm totally OK deferring. I mainly just wanted to document that other branch here so we could move forward without duplicating too much effort. Since my branch implements it incorrectly, however, it's no longer clear if adopting it would buy us much.