-
Notifications
You must be signed in to change notification settings - Fork 37
CQL Long #363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
CQL Long #363
Changes from all commits
8bcfa6c
62f9c44
219d086
b6c8069
ecaa61d
11a0168
451763e
2d21c16
5e4472f
bb6e72c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| // By default, BigInt throws a TypeError if you attempt to JSON.stringify it. | ||
| // You can avoid the TypeError by defining a `toJSON()` function for BigInt. | ||
| // We will use the same serialization approach as FHIR uses for integer64: a string. | ||
| // See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#use_within_json | ||
| // See: https://hl7.org/fhir/R5/json.html#primitive | ||
|
|
||
| declare global { | ||
| interface BigInt { | ||
| toJSON?(): unknown; | ||
| } | ||
| } | ||
|
|
||
| if (BigInt.prototype.toJSON === undefined) { | ||
| BigInt.prototype.toJSON = function () { | ||
| return this.toString(); | ||
| }; | ||
| } | ||
|
|
||
| // empty export forces typescript to recognize this file as a module | ||
| export {}; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| export * from './bigint'; | ||
| export * from './logic'; | ||
| export * from './clinical'; | ||
| export * from './uncertainty'; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,6 +10,15 @@ import { | |||||
| minValueForType | ||||||
| } from '../util/math'; | ||||||
| import * as cmp from '../util/comparison'; | ||||||
| import { | ||||||
| ELM_INTEGER_TYPE, | ||||||
| ELM_DECIMAL_TYPE, | ||||||
| ELM_LONG_TYPE, | ||||||
| ELM_TIME_TYPE, | ||||||
| ELM_DATE_TYPE, | ||||||
| ELM_DATETIME_TYPE, | ||||||
| ELM_QUANTITY_TYPE | ||||||
| } from '../util/elmTypes'; | ||||||
|
|
||||||
| export class Interval { | ||||||
| constructor( | ||||||
|
|
@@ -32,17 +41,17 @@ export class Interval { | |||||
| const point = this.low != null ? this.low : this.high; | ||||||
| if (point != null) { | ||||||
| if (typeof point === 'number') { | ||||||
| pointType = Number.isInteger(point) | ||||||
| ? '{urn:hl7-org:elm-types:r1}Integer' | ||||||
| : '{urn:hl7-org:elm-types:r1}Decimal'; | ||||||
| pointType = Number.isInteger(point) ? ELM_INTEGER_TYPE : ELM_DECIMAL_TYPE; | ||||||
| } else if (typeof point === 'bigint') { | ||||||
| pointType = ELM_LONG_TYPE; | ||||||
| } else if (point.isTime && point.isTime()) { | ||||||
| pointType = '{urn:hl7-org:elm-types:r1}Time'; | ||||||
| pointType = ELM_TIME_TYPE; | ||||||
| } else if (point.isDate) { | ||||||
| pointType = '{urn:hl7-org:elm-types:r1}Date'; | ||||||
| pointType = ELM_DATE_TYPE; | ||||||
| } else if (point.isDateTime) { | ||||||
| pointType = '{urn:hl7-org:elm-types:r1}DateTime'; | ||||||
| pointType = ELM_DATETIME_TYPE; | ||||||
| } else if (point.isQuantity) { | ||||||
| pointType = '{urn:hl7-org:elm-types:r1}Quantity'; | ||||||
| pointType = ELM_QUANTITY_TYPE; | ||||||
| } | ||||||
| } | ||||||
| if (pointType == null && this.defaultPointType != null) { | ||||||
|
|
@@ -288,7 +297,7 @@ export class Interval { | |||||
| !other.highClosed && | ||||||
| !this.highClosed) | ||||||
| ) { | ||||||
| if (typeof this.low === 'number') { | ||||||
| if (typeof this.low === 'number' || typeof this.low === 'bigint') { | ||||||
| if (!(this.start() === other.start())) { | ||||||
| return false; | ||||||
| } | ||||||
|
|
@@ -302,7 +311,7 @@ export class Interval { | |||||
| (this.low == null && other.low != null && this.high != null && other.high != null) || | ||||||
| (this.low == null && other.low == null && this.high != null && other.high != null) | ||||||
| ) { | ||||||
| if (typeof this.high === 'number') { | ||||||
| if (typeof this.high === 'number' || typeof this.high === 'bigint') { | ||||||
| if (!(this.end() === other.end())) { | ||||||
| return false; | ||||||
| } | ||||||
|
|
@@ -337,7 +346,7 @@ export class Interval { | |||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| if (typeof this.low === 'number') { | ||||||
| if (typeof this.low === 'number' || typeof this.low === 'bigint') { | ||||||
| return this.start() === other.start() && this.end() === other.end(); | ||||||
| } else { | ||||||
| return ( | ||||||
|
|
@@ -497,6 +506,8 @@ export class Interval { | |||||
| let diff = Math.abs(highValue - lowValue); | ||||||
| diff = Math.round(diff * Math.pow(10, 8)) / Math.pow(10, 8); | ||||||
| return new Quantity(diff, closed.low.unit); | ||||||
| } else if (typeof closed.low === 'bigint') { | ||||||
| return closed.high >= closed.low ? closed.high - closed.low : closed.low - closed.high; | ||||||
| } else { | ||||||
| // TODO: Fix precision to 8 decimals in other places that return numbers | ||||||
| const diff = Math.abs(closed.high - closed.low); | ||||||
|
|
@@ -528,6 +539,9 @@ export class Interval { | |||||
| let diff = Math.abs(highValue - lowValue) + pointSize.value; | ||||||
| diff = Math.round(diff * Math.pow(10, 8)) / Math.pow(10, 8); | ||||||
| return new Quantity(diff, closed.low.unit); | ||||||
| } else if (typeof closed.low === 'bigint') { | ||||||
| const diff = closed.high >= closed.low ? closed.high - closed.low : closed.low - closed.high; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to above: I'm not sure why we have the ternary here. Shouldn't it just be closed.high - closed.low ()
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, I was using the ternary since |
||||||
| return diff + 1n; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally
Suggested change
according to https://cql.hl7.org/04-logicalspecification.html#size
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think I was doing a shortcut because if we know it is a Long ( |
||||||
| } else { | ||||||
| const diff = Math.abs(closed.high - closed.low) + pointSize.value; | ||||||
| return Math.round(diff * Math.pow(10, 8)) / Math.pow(10, 8); | ||||||
|
|
@@ -556,8 +570,8 @@ export class Interval { | |||||
| throw new Error('Point type of intervals cannot be determined.'); | ||||||
| } | ||||||
|
|
||||||
| if (typeof pointSize === 'number') { | ||||||
| pointSize = new Quantity(pointSize, '1'); | ||||||
| if (typeof pointSize === 'number' || typeof pointSize === 'bigint') { | ||||||
| pointSize = new Quantity(Number(pointSize), '1'); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think ideally we'd want the pointsize for bigint to be 1n. The
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You know, I feel like I had a good reason for this -- like the spec didn't define a Or maybe this was just a straight-up mistake. Anyway, I agree with you. |
||||||
| } | ||||||
|
|
||||||
| return pointSize; | ||||||
|
|
@@ -610,7 +624,11 @@ function areDateTimes(x: any, y: any) { | |||||
|
|
||||||
| function areNumeric(x: any, y: any) { | ||||||
| return [x, y].every(z => { | ||||||
| return typeof z === 'number' || (z != null && z.isUncertainty && typeof z.low === 'number'); | ||||||
| return ( | ||||||
| typeof z === 'number' || | ||||||
| typeof z === 'bigint' || | ||||||
| (z != null && z.isUncertainty && (typeof z.low === 'number' || typeof z.low === 'bigint')) | ||||||
| ); | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we have the ternary here. Shouldn't it just be
closed.high - closed.lowaccording to https://cql.hl7.org/04-logicalspecification.html#widthThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The normal
number-based representation on line 513 usesMath.abs(closed.high - closed.low). SinceMath.absdoesn't work withbigint, I was using the ternary to get the absolute value instead (and more accurately mirror the number-based representation).But you're right. Neither is really necessary, because the authoring guide says:
So
closed.high - closed.lowis always a positive value anyway.