Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
+ Coverage 87.69% 88.25% +0.56%
==========================================
Files 52 54 +2
Lines 4624 4802 +178
Branches 1307 1349 +42
==========================================
+ Hits 4055 4238 +183
- Misses 354 371 +17
+ Partials 215 193 -22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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?
|
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! |
|
FYI: I rebased this branch on master to take in the latest |
lmd59
left a comment
lmd59
left a comment
There was a problem hiding this comment.
Excellent updates, and addressing a lot of new Long functionality and improved type handling!
I added some comments, but most of them are small!
|
Fantastic feedback again, @lmd59. You're making me feel like I'm getting sloppy. Thanks for the thorough review and great finds. I'll fix these soon (but probably not today). |
- 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
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
Co-authored-by: lmd59 <lmd59@cornell.edu>
- Remove unnecessary abs and ternaries in Interval.width() and Interval.size() - Fix possible overflows in Interval.getPointSize() - Refactor Interval.getPointSize() to return Quantity | Number | BigInt - Update Product CQL tests to pass a second argument - Remove redundant interval tests and clarify test names where appropriate
Rebase mistakenly duplicated types in compilerOptions.
- Modulo divide by zero should return null - Abs and Negate can overthrow when minimum integer or long are passed in - ToInteger and ToLong should return null for empty and invalid strings - Account for incorrect resultTypeName (from translator) in overflowsOrUnderflows
lmd59
left a comment
lmd59
left a comment
There was a problem hiding this comment.
Just one tiny question - looking great to me! Excited that this will be supported soon!
Co-authored-by: lmd59 <lmd59@cornell.edu>
dehall
left a comment
dehall
left a comment
There was a problem hiding this comment.
This looks good. I came at this primarily from the perspective of how one might build off this for a future "BigDecimal"-based type and I feel like this is a good model to follow. One or maybe two spots I might have refactored some duplicate logic into a helper function but mixing JS primitives with classes means that's not really possible very often.
In the interest of understanding what our modern AI tools are capable of I did ask Codex to see what it thought, and it highlighted a few items which I confirmed. I think the 2 Uncertainty code suggestions I made are necessary but I defer to you on what you want to do for everything else
| return val.high; | ||
| } | ||
| })(); | ||
| return new Uncertainty(successor(val.low), high); |
There was a problem hiding this comment.
Should type be passed in here as well? Same below for predecessor
| return new Uncertainty(successor(val.low), high); | |
| return new Uncertainty(successor(val.low, type), high); |
On a related note, I don't see any tests that seem like they would hit this branch. I wasn't immediately sure what the best place for one was was but this quickly hacked together test (ostensibly test/util/math-test.ts) shows the issue:
import should from 'should';
import { Uncertainty } from '../../src/datatypes/uncertainty';
import { successor, predecessor } from '../../src/util/math';
import { ELM_DECIMAL_TYPE, ELM_INTEGER_TYPE } from '../../src/util/elmTypes';
describe('successor', () => {
it('should preserve integers in an Uncertainty', () => {
const result = successor(new Uncertainty(1.0, 2.0), ELM_INTEGER_TYPE);
result.low.should.equal(2);
result.high.should.equal(3);
});
it('should preserve decimals in an Uncertainty', () => {
const result = successor(new Uncertainty(1.0, 2.0), ELM_DECIMAL_TYPE);
result.low.should.equal(1.00000001);
result.high.should.equal(2.00000001);
});
});
describe('predecessor', () => {
it('should preserve integers in an Uncertainty', () => {
const result = predecessor(new Uncertainty(1.0, 2.0), ELM_INTEGER_TYPE);
result.low.should.equal(0);
result.high.should.equal(1);
});
it('should preserve decimals in an Uncertainty', () => {
const result = predecessor(new Uncertainty(1.0, 2.0), ELM_DECIMAL_TYPE);
result.low.should.equal(0.99999999);
result.high.should.equal(1.99999999);
});
});There was a problem hiding this comment.
Good catch. I thought I did a global find/replace for successor/predecessor, but apparently I missed some. I also thought I did a full Codex review of my own code, but maybe I made the successor/predecessor change after that (or maybe Codex just gave you different findings than it gave me, which I've definitely seen before).
| return val.low; | ||
| } | ||
| })(); | ||
| return new Uncertainty(low, predecessor(val.high)); |
There was a problem hiding this comment.
Same as above - should type be passed in here as well?
| return new Uncertainty(low, predecessor(val.high)); | |
| return new Uncertainty(low, predecessor(val.high, type)); |
| const results = []; | ||
| while (current_high <= longHigh) { | ||
| results.push(new dtivl.Interval(current_low, current_high, true, true)); | ||
| current_low += perBigInt; |
There was a problem hiding this comment.
I realize it's invalid for the per value on an interval to be 0, but it does appear to be technically possible to reach here with perBigInt = 0.
eg:
define ZeroPer: expand { Interval[2 'g', 4 'g'] } per 0 'g'
This then becomes an infinite loop (eventually crashing, out of memory, probably because of results growing too large). I can't find anything obvious in the spec on what to do in this situation. https://cql.hl7.org/09-b-cqlreference.html#expand does mention null as a possibility but not 0. Since there are already a few early exits to this function that return [], if there's no formal guidance on what to do I'd propose just adding a 0 check in there as well, but I'm open to anything.
| } else if (typeof val === 'number') { | ||
| return new Quantity(val, '1'); | ||
| } else if (typeof val === 'bigint') { | ||
| return new Quantity(Number(val), '1'); |
There was a problem hiding this comment.
Super minor: you could have a comment here that converting a BigInt to a Number does theoretically have some loss of precision but because the range of safe integers is so big*, it's probably not a big deal in the context of CQL Quantity
* = -(2^53 - 1) to 2^53 - 1, (±9,007,199,254,740,991)
(aka plus or minus 9 quadrillion, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isSafeInteger#description )
There was a problem hiding this comment.
If I'm making changes in the code already, then I might as well. This will likely become a moot point once we introduce a class to handle Decimals better.
4 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: