Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
+ Coverage 87.58% 87.87% +0.28%
==========================================
Files 52 53 +1
Lines 4606 4683 +77
Branches 1297 1301 +4
==========================================
+ Hits 4034 4115 +81
- Misses 359 373 +14
+ Partials 213 195 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add support for Long literal, ToLong conversion, min/max long values - Use JavaScript Number to represent Long (NOTE: this means that values are imprecise outside of the safe integer range in JS) - Add tests for literals, conversion, and other operations that accept Long arguments - Improve underflow/overflow tests to test boundaries more carefully - Added .skip to tests that fail due to Number imprecision for high values - Unskipped Long tests in the spec tests that now pass
Update support for Long to use BigInt so we can distinguish between decimal/integer (JS Number) and long (JS BigInt).
- support for <, <=, >=, > for long/bigint types - support for ConvertsToLong and Convert operator with Long
lmd59
left a comment
lmd59
left a comment
There was a problem hiding this comment.
Looking good! The tests are very thorough and helpful for confirming successful implementation. I left a couple of small comments on tests. Most are just nitpick/optional, but I think a couple more aggregate tests wouldn't hurt.
I also wanted to ask about support for Long in the interval datatype
https://cql.hl7.org/04-logicalspecification.html#interval states that "An interval must be defined using a point type that supports comparison, as well as Successor and Predecessor operations, and Minimum and Maximum Value operations." Long seems like it would fit in this category. Is there any reason not to support Long as part of an interval?
| (await this.intOne.exec(this.ctx)).should.equal(1); | ||
| }); | ||
|
|
||
| it('should convert 1L to 1', function () { |
There was a problem hiding this comment.
Would this be a little more clear?
| it('should convert 1L to 1', function () { | |
| it('should convert 1L to 1n', function () { |
| this.longOne.value.should.equal(1n); | ||
| }); | ||
|
|
||
| it('should execute 1L as 1', async function () { |
There was a problem hiding this comment.
Would this be a little more clear?
| it('should execute 1L as 1', async function () { | |
| it('should execute 1L as 1n', async function () { |
| define LongDivideUnderflow: minimum Long / 0.5 | ||
| define LongDivideNearOverflow: ToLong(maximum Integer) / 1L | ||
| define LongDivideNearUnderflow: ToLong(minimum Integer) / 1L | ||
| define LongDivideByZero: 1L / 0L |
There was a problem hiding this comment.
Would it add anything to consider a long/integer mixed divide by zero?
| // @Test: Product | ||
| define decimal_product: Product({1.0, 2.0, 3.0, 4.0}) | ||
| define integer_product: Product({5, 4, 5}) | ||
| define long_product: Product({5L, 4L, 5L}) |
There was a problem hiding this comment.
Recommend adding a test for over/underflow on an aggregate product.
|
|
||
| // @Test: Sum | ||
| define not_null: Sum({1,2,3,4,5}) | ||
| define longs: Sum({1L,2L,3L,4L,5L}) |
There was a problem hiding this comment.
Good test for aggregate functionality. Would also recommend adding a test for over/underflow on an aggregate sum.
3 participants
This PR introduces support for CQL Long, represented using
BigIntin JavaScript. Since Integer and Decimal are represented using Number, we can easily distinguish CQL Longs from Integers and Decimals by their type (typeof var === 'bigint'). This actually made implementation easier (and more correct) compared to my initial implementation using Number (which I thought would be easier since it was more of an incremental change, but maybe not).You can test this by building and running a CQL library that uses Long types -- or you can just review the unit tests and trust that they are properly exercising the capability.
Submitter:
npm run test:plusto run tests, lint, and prettier)cql4browsers.jsbuilt withnpm run build:browserifyif source changed.Reviewer:
Name: