Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
+ Coverage 87.58% 88.06% +0.48%
==========================================
Files 52 54 +2
Lines 4606 4760 +154
Branches 1297 1327 +30
==========================================
+ Hits 4034 4192 +158
- Misses 359 372 +13
+ Partials 213 196 -17 ☔ 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?
| // @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.
Co-authored-by: lmd59 <lmd59@cornell.edu>
- Add optional resultTypeName property to base Expression class
- Use resultTypeName when possible to distinguish between decimal and integer
- Use constants for ELM type strings (e.g., {urn:hl7-org:elm-types:r1}Decimal)
- Update spec-test-data generator CQL-to-ELM to produce resultTypes
- Unskip tests that are now passing
- Fix tests that were unintentionally incorrect based on previous incomplete type support
|
Thanks for the excellent review! You uncovered some gaps in my implementation and also helped me to find more gaps on my own. But I will also note: be careful what you wish for! I just added a lot more changes and tests.
Thanks again for the great review. Sorry for adding a bunch more work for your review! |
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: