Improved support for CQL Decimal

[dehall]

This PR migrates Decimals from being represented by plain JS number to being represented by a Decimal class. (CQL Integers remain represented by plain JS numbers.) Our new Decimal class is a wrapper around the decimal.js library. All interactions with the library are limited to one file so if we decide that's the wrong library, it should be straightforward to change.

Where possible, I've tried to make the changes developer-friendly, for instance, because Quantity.value is always a Decimal, the Quantity constructor accepts anything that can be converted to a Decimal, eg, a number, bigint, or string. This is primarily relevant to the unit tests where we have a lot of "result should equal(new Quantity(3, 'g')" style test expectations.

This change means that now all CQL types are represented 1:1 by their respective JS types, so passing a "type" argument around became unnecessary in several places.

Decimals need to be normalized to a max of 8 digits after the decimal point, and have a maximum and minimum value, and so my philosophy was to try to normalize and bounds check as few times as possible, and hence as late as possible: only in the ELM layer. There are some remaining instances of checking for overflow in the datatype layer that I could have removed, but that would require even more refactoring so I left them for now.

Per the spec, the string representation of a Decimal must always contain at least one digit on either side of the decimal point. (eg, 1.0 not 1 or 1., and 0.1 not .1, and not exponential notation like 1e8)

Changes here mostly fall into 3 categories:

• Arithmetic operator support
• Aggregate operator support
• Interval expansion
  • Now instead of separate logic for number/bigint/Decimal, interval expansion is always performed with Decimals and converted back to the appropriate type when each "sub-interval" is constructed
  • While looking at interval expansion, I saw some skipped spec tests that could be fixed by adding just a few lines, so true support for the "single interval" expand overload is now present

Note this PR does not implement the CQL Precision operator, and the Decimal class doesn't keep track of significant figures. (JS numbers didn't either, so this isn't a regression) This means that trailing zeros after the decimal point will not be preserved. eg:

Decimal.from("1.00000000").toString()
-->
"1.0"

decimal.js doesn't support this natively, so a future effort will have to add a second internal state field to track scale.

The two unit test failures are expected at this point (I removed the part of the expand Interval logic that covers those 2 specific tests) but I'm waiting for more direction on #cql > Interval Expand example before doing anything more on that front.

Notable Boundaries

There are a couple instances where interactions with plain JS numbers are forced:

• Quantity operations that interact with the UCUM unit conversion library
  • To try to reduce the chance of loss of precision, instead of calling the library with "n of unit A to unit B", I call the library with "1 of unit A to unit B" and then multiplying the input Decimal by the resulting factor.
• DateTime operations that interact with the library luxon, specifically the timezoneOffset field
  • I think these always should be integers or rational numbers in a small range, so I'm pretty sure every possible value can be exactly represented with number anyway

Also note that the Decimal constructor accepts JS numbers that do not need to represent integers, but there is the risk of loss of precision if the literal used cannot be represented precisely as a js number. To be safe, consumers of this library constructing a Decimal instance should generally use the string constructor which guarantees round-trip safety. Eg:

Decimal.from(1.0000000000000000000000000001).toString()
--> "1"
 
Decimal.from("1.0000000000000000000000000001").toString()
--> "1.0000000000000000000000000001"

Decimal.from(900719925474099230945).toString()
--> "900719925474099200000" 

Decimal.from("900719925474099230945").toString()
--> "900719925474099230945"

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 check to run tests, lint, and prettier)
• All dependent libraries are appropriately updated or have a corresponding PR related to this change
  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 92.02128% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.23%. Comparing base (81b47fc) to head (4800e97).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/elm/arithmetic.ts	87.20%	6 Missing and 5 partials ⚠️
src/elm/interval.ts	83.05%	4 Missing and 6 partials ⚠️
src/datatypes/decimal.ts	93.91%	5 Missing and 2 partials ⚠️
src/util/math.ts	94.00%	3 Missing and 3 partials ⚠️
src/util/immutableUtil.ts	50.00%	2 Missing and 1 partial ⚠️
src/datatypes/datetime.ts	88.88%	1 Missing and 1 partial ⚠️
src/datatypes/interval.ts	88.88%	0 Missing and 2 partials ⚠️
src/elm/aggregate.ts	97.18%	1 Missing and 1 partial ⚠️
src/datatypes/quantity.ts	95.23%	1 Missing ⚠️
src/elm/type.ts	95.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   88.70%   89.23%   +0.52%     
==========================================
  Files          59       60       +1     
  Lines        4933     5119     +186     
  Branches     1429     1469      +40     
==========================================
+ Hits         4376     4568     +192     
+ Misses        322      316       -6     
  Partials      235      235              

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

Wow. This is a pretty intense PR! It's great to finally have more reasonable support for decimals and decimal-based operations. I've left some comments about some mostly small things. I think the biggest thing is I would like for us to support retention of decimal precision -- but I'm totally fine with doing that as a follow-on to this PR (rather than trying to fit it into this PR).

The other big question -- which is definitely a "for later" thing -- is if we might gain some simplicity by modeling all numeric types as CQL classes (e.g., Integer, Long, and Decimal). I think they could all be backed by decimal.js, and if they had a common base class and/or implemented a common interface it might simplify things a lot. This is just a half-baked idea, but I think it may be worth exploring -- especially if we're going to do a major release w/ breaking changes anyway.

One other thing to note: I'm guessing we may need to update cql-exec-fhir to account for the new Decimal class. Maybe after we review and merge this PR, we should do a 4.0.0-beta.1 release so we can try integrating it w/ a cql-exec-fhir beta release as well (which both will probably be needed to update / test fqm-execution). I know I said I wanted to target Connectathon for a release of this stuff, but I think a beta release would be fine (I feel ok reporting against a beta release if it's on the main branch).

[dehall]

Just pushed a series of commits based on some further testing:

• Added Decimal.truncatedDivideBy because just using regular divideBy then truncating meant that the value was first rounded, which produced incorrect results. To call this as appropriate, I added a new optional parameter in "MathUtil" divide to decide if you want truncated divide or not
• Added Decimal.truncateToBigInt as a companion to Decimal.truncate which returns a number. This is only actually used in Interval expand, but it could theoretically be used in the Truncate operation in the future
• Add Decimal.nthRoot to try to allow for more accurate results in some cases. Ideally, Decimal.power would be enough, but the way decimal.js implements the power operator can result in imprecise values when the power is a fraction like 1/3. This was already visible in examples like GeometricMean({4.0, 4.0, 4.0 }). For now the only special cases in nthRoot are 2 (square root, calls .sqrt), and 3 (cube root, calls decimal.js cubeRoot), otherwise it just defers to power as before.
• Made the parameter in Decimal.round optional. If it's omitted or null, the spec says treat it as if 0 were passed in.
• Updated the NormalizedKey definition for Decimals and used that key in Mode logic, so that it allows value equality. Since Decimal value equality ignores trailing zeros, that's done here.

I'm going to continue poking at this but as of now I'm calling it officially ready for re-review.

[cmoesel]

This is an impressive PR. Who knew decimals would affect so much! Thanks for working through it. I think we're super close.

I've only left a few small comments (some of them just to vent about things that annoy me in CQL). I also noticed that if I run npm run check:all some of the package-lock.json files change (based on the introduction of the decimal.js library).

Let me know if you want to discuss any of this!

[cmoesel]

Not a big deal, but I can't figure out how you decide what your line length / line break strategy is on some of these comments. You don't have to change it, but I'm curious how you decide (for example, why break at column 53 on line 174, but column 78 on line 176?

[dehall]

Not a super well-defined strategy. I wouldn't bother for, say, typesetting a whole page of text, but for comments like this that are just a few lines I try to keep a whole clause or whole idea on one line. (This sentence comes from the spec, if I wrote it myself I'd try to fit one full idea per line. I might add quotes around it now to indicate it's from the spec.) Here I probably added an intentional line break at the semicolon, then tried to figure out how I could make the parts before and after fit within our line length

[cmoesel]

Should we also test for invalid scale (e.g., a negative number as the scale)?

[dehall]

The only places that a caller can pass in arbitrary scale are round, withScale, and withMinimumScale - Decimal.from() doesn't take one, and the constructor is private so it's dependent on the other methods to pass in correct values. (I think the logic on the others should be safe but if you see something definitely call it out). I have a couple invalid scale examples for withScale but I'll add some for the others too

[cmoesel]

This is correct (based on "trailing zeroes after the decimal are ignored in determining precision for equivalent comparison."), but just for the record, I don't like it! The spec generally says that precision matters, and I feel like if an input says 1.20 rather than 1.2, that means they measured to the hundredths -- and based on that, the fact that the hundredths place holds a 0 should not matter.

For example, 1.00000000 ~ 1.499999999 is true? That's ridiculous. If someone is using instruments to measure to the hundred-millionths, then obviously those small differences matter. We should not ignore them because the value happens to be even at 0s. I get why 1.00 ~ 1.00499999, because one number was measured at a different scale to begin with, but... 1.00000000 is a different story.

OK. Getting off my soapbox now. Again, there is nothing to do here. I just needed to vent.

[cmoesel]

I'm not sure about this one. The description of Except does not indicate anything about precision, nor does it define itself in terms of successor, predecessor, start, or end. We implemented it using some of those concepts, but that doesn't mean that was the correct way to implement it (and notably had different results since predecessor/successor always used .00000001 on decimals).

How did you determine that precision of the input intervals should be used here? I think we should request that this be clarified in the spec.

[cmoesel]

I agree that this is wrong based on the current spec, but it is also wrong for the string representation to introduce a precision that is not there in the input. I know you already have raised that elsewhere; this is just another case where it matters.

For quantities, it's actually a little easier because the context itself tells you that the number is a decimal (since quantity values are always decimals), so 1 'cm' is unambiguously a decimal with scale 0 (as opposed to a bare number like 1).

[cmoesel]

I disagree with the expected answer in this test. I think our answer [0, 1] is correct because @T06 represents an imprecise time between @T06:00:00.0 and @T06:59:59.999, so hours between @T06 and @T07:00:00 is only 1 when @T06 is @T06:00:00.0, but it is 0 for all other potential values of @T06. So... I think this should be moved to the section of the skip list for wrong expected outputs.

[dehall]

Sounds good. Sorry about these, I'm pretty sure these were existing tests that were unintentionally not being tested because earlier tests in the file had (intentional) parse errors. Skipping those parse error tests revealed these, and once it was apparent they weren't related to Decimal I didn't look much closer and just skipped them.

[cmoesel]

I hate this one. The test is:

DateTime(2016,5) - 31535999 seconds = DateTime(2015, 5)

and they expect true.

But... let's do the math: 60*60*24*365 = 31,536,000. The CQL above uses 31535999, which is one second short of a full year. This is why we say 2015-06. Never mind the fact that 2016 was a leap year, so really there is a whole extra day of seconds (so 31535999 is actually 86401 seconds short of that particular year span).

Unfortunately, they arrive at their answer because they convert 31535999 seconds to months and use (the very imprecise) 60*60*24*30 to determine how many seconds are in a month -- so 31535999 / 60*60*24*30 = 12.16666628, and hence they arrive at 2015-05.

I've argued that if we need a consistent conversion factor for months it should be 60*60*24*365/12 (which corrects this example so that 31535999 / 60*60*24*30 = 11.99999962), but based on the language in Equal, I've lost that argument.

Anyway, we're right to skip it for now, and we should probably fix the engine to use 30-day months, but I really don't want to.

[cmoesel]

Not a big deal, but so far we have only used " when the test name has a space in it.

[dehall]

This does have a space in the test group name (Truncated Divide)

[cmoesel]

Doh. I looked at that like 5 times!