Improved support for CQL Decimal - #376
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
==========================================
+ Coverage 88.70% 89.14% +0.43%
==========================================
Files 59 60 +1
Lines 4933 5104 +171
Branches 1429 1469 +40
==========================================
+ Hits 4376 4550 +174
+ Misses 322 318 -4
- Partials 235 236 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dehall
changed the title
cmoesel
left a comment
cmoesel
left a comment
There was a problem hiding this comment.
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).
| // TODO: predecessor should be based on current precision | ||
| // For Decimal, predecessor is equivalent to subtracting 1 * the precision of the argument. | ||
| return new Decimal(this.value.minus(MIN_PRECISION_VALUE)); | ||
| } |
There was a problem hiding this comment.
Supporting precision (and operations that depend on it) is important. It's one of the reasons I wanted to move away from Number. We'll have to figure out a way to support this. I was thinking that we might be able to support it if we stored this.value as a string -- but we probably need to think about it further to see if that would really work. I also asked ChatGPT about this and it recommended tracking scale as a separate value in our datatype (much like you suggested in the PR description).
There was a problem hiding this comment.
From offline discussion: I'll implement precision in this PR. Preserving the precision of a literal is straightforward, but the spec doesn't define how to handle precision across arithmetic operators. Eg, what should Precision(1.000000 + 1.0) be? I'll use the kotlin reference impl/cql playground for inspiration
| // ROUND_HALF_CEIL "Rounds towards nearest neighbour. If equidistant, rounds towards Infinity" | ||
| // rounds 0.5 -> 1.0, -0.5 -> 0.0 | ||
| // https://mikemcl.github.io/decimal.js/#modes | ||
| return this.setScale(scale, CQLDecimalJS.ROUND_HALF_CEIL); |
There was a problem hiding this comment.
CQL 2.0 clarified rounding semantics in response to FHIR-45987:
The semantics of round are defined as a traditional round (i.e. to the nearest whole number), meaning that a decimal value greater than or equal to 0.5 and less than 1.0 will round to 1, and a decimal value less than or equal to -0.5 and greater than -1.0 will round to -1. [1.9.6.19 Round]
I'm not sure if that's really a "traditional round", but the example indicates that -0.5 rounds to -1, so it seems like we really ought to be using CQLDecimalJS.ROUND_HALF_UP now.
There was a problem hiding this comment.
From offline discussion: this was wrong based on trying to pass some cql-tests. Those have been updated upstream so I'll pull the latest cql-tests and update elsewhere as appropriate
| return this.setScale(scale, CQLDecimalJS.ROUND_HALF_CEIL); | ||
| } | ||
|
|
||
| setScale(scale: number, roundingMode: DecimalRoundingMode = CQLDecimalJS.ROUND_DOWN) { |
There was a problem hiding this comment.
Since CQL Round supports passing in a precision, and since it specifies rounding away from zero, I think it probably makes sense to use the same rounding algorithm when reducing scale generally. That is unless you saw something else in the spec that would indicate otherwise (which is totally possible).
| // note that this is permissive and converts non-integral values | ||
| return this.truncate(); |
There was a problem hiding this comment.
Why are we permissive on this? And why truncate (vs. round)? Wouldn't we want 1.99999999 to go to 2)?
There was a problem hiding this comment.
From offline discussion: will remove this function and toLong below, since they aren't used for any ELM expressions
| // note that this is permissive and converts non-integral values | ||
| return BigInt(this.value.truncated().toString()); |
There was a problem hiding this comment.
Same question as for toInteger:
Why are we permissive on this? And why truncate (vs. round)? Wouldn't we want 1.99999999 to go to 2)?
| const lte = (a: any, b: any): boolean | null => { | ||
| if (typeof a !== typeof b || a?.constructor !== b?.constructor) { | ||
| return null; | ||
| } | ||
|
|
||
| if (typeof a.sameOrBefore === 'function') { | ||
| return a.sameOrBefore(b); | ||
| } else { | ||
| return a <= b; | ||
| } | ||
| }; | ||
| const gte = (a: any, b: any): boolean | null => { | ||
| if (typeof a !== typeof b || a?.constructor !== b?.constructor) { | ||
| return null; | ||
| } | ||
|
|
||
| if (typeof a.sameOrBefore === 'function') { | ||
| return a.sameOrAfter(b); | ||
| } else { | ||
| return a >= b; | ||
| } | ||
| }; |
There was a problem hiding this comment.
These functions need to be updated to support decimals since <= and >= might not work predictably with our Decimal class.
| return val && val.isDecimal; | ||
| case ELM_INTEGER_TYPE: | ||
| return typeof val === 'number' && Math.floor(val) === val; | ||
| return typeof val === 'number'; |
There was a problem hiding this comment.
Should we still do some checking to ensure it really is an integer (and not 3.5 or NaN or Infinity)? I don't know if it's possible for one of those values to get in here, but it might be safest to do the check (like we were doing before).
| return val && val.isDecimal; | ||
| } else if (inst.isIntegerLiteral) { | ||
| return typeof val === 'number' && Math.floor(val) === val; | ||
| return typeof val === 'number'; |
There was a problem hiding this comment.
Should we still do some checking to ensure it really is an integer (and not 3.5 or NaN or Infinity)? I don't know if it's possible for one of those values to get in here, but it might be safest to do the check (like we were doing before).
| isUTC() { | ||
| // A timezoneOffset of 0 indicates UTC time. | ||
| return !this.timezoneOffset; | ||
| } |
There was a problem hiding this comment.
This needs to be updated to check equality with Decimal.from(0) now. As it is, no populated timezoneOffset will every register as UTC since !this.timezoneOffset will always be false for an instance of Decimal.
| const sum = values.reduce((x, y) => x + y); | ||
| return overflowsOrUnderflows(sum, ELM_DECIMAL_TYPE) ? null : new Quantity(sum, items[0].unit); | ||
| // note doAddition is Quantity addition | ||
| sum = items.reduce(doAddition); |
There was a problem hiding this comment.
Since doAddition checks for overflow/underflow, that means we'll null out if any intermediate value goes beyond CQL's min/max values, even if the final result is within the min/max. This also means that the order of the input values can affect the results (since re-ordering the numbers can sometimes avoid an intermediate overflow, e.g., (MAX 'm', 1 'm', -2 'm') vs (MAX 'm', -2 'm', 1 'm')).
Maybe we shouldn't reject intermediate values that we're capable of processing if they don't ever get represented as a CQL type at an operation boundary. The previous implementation (that just used JS +) did not check intermediate values, nor does the current implementation for decimals (which uses sumOfDecimals).
3 participants
This PR migrates Decimals from being represented by plain JS
numberto being represented by aDecimalclass. (CQL Integers remain represented by plain JS numbers.) Our newDecimalclass 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.valueis always a Decimal, theQuantityconstructor 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.0not1or1., and0.1not.1, and not exponential notation like1e8)Changes here mostly fall into 3 categories:
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.jsdoesn'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 Intervallogic 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:
luxon, specifically the timezoneOffset fieldnumberanywayAlso 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:
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 checkto run tests, lint, and prettier)Reviewer:
Name: