Interval Operator Overhaul#373
Conversation
…tors - Redefine Start and End operators based on clarified definitions in upcoming CQL 2.0 - Update interval operators to follow CQL spec implementation language as closely as possible - Preserve interval point types and improve arithmetic, precision, and limit handling - Expand unit, integration, and specification test coverage - Reorganize some exports and imports to avoid circular imports - NOTE: expand and collapse implementations have not yet been updated as they are considerably more complex
The logical definition and reference definition for proper contains/includes disagree. The reference definition seems more consistent to me, so I've implemented that while we ask the CQL community.
|
@lmd59 - I added you as a reviewer since you reviewed #365 (which this replaces), but if you don't have time, then I think anyone else on the team would be fine. @dehall will also review, but this one is kind of big, so two reviewers might be good -- especially if someone from your team can regress it with fqm. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #373 +/- ##
==========================================
+ Coverage 88.41% 89.48% +1.07%
==========================================
Files 54 55 +1
Lines 4832 4843 +11
Branches 1359 1390 +31
==========================================
+ Hits 4272 4334 +62
+ Misses 368 321 -47
+ Partials 192 188 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dehall
left a comment
dehall
left a comment
There was a problem hiding this comment.
I haven't had time to fully finish reviewing but overall it looks good. Submitting the few comments I do have now, since I'll be out Monday
| return 999; | ||
| default: | ||
| throw new Error('Tried to clieling a field that has no cieling value: ' + field); | ||
| throw new Error('Tried to ceiling a field that has no cieling value: ' + field); |
There was a problem hiding this comment.
Are you open to fixing the name of this whole function while you're here?
| "CqlArithmeticFunctionsTest.Truncated Divide.TruncatedDivide414By206DQuantity" Truncated divide not implemented for Quantity | ||
|
|
||
| # Unimplemented (New in CQL 2.0) | ||
| CqlListOperatorsTest.Slice Slice not implemented No newline at end of file |
There was a problem hiding this comment.
This looks like an incorrect change by the check-skip-list script, similar to what happened to me on a recent PR. The cql-to-elm-cli failed to generate ELM for CqlListOperatorsTest.cql but doesn't report the error by a nonzero exit code, and so the check-skip-list task happily continues, runs the tests against the old ELM, and thinks the new tests are 100% passing, so a test that really should be skipped gets un-skipped.
The other way to notice this is that this PR currently has a modified test/spec-tests/cql/CqlListOperatorsTest.cql but not a corresponding CqlListOperatorsTest.json. Shoutout to @lmd59 for catching that on my earlier PR so now I know what to look for. I have a PR up on the translator CLI to help here, hopefully that gets some eyes on it soon.
Turns out, this specific Slice test case is what's causing the translation to fail:
================================================================================
TRANSLATE /Users/dehall/cql-execution-review/test/generator/../spec-tests/cql/CqlListOperatorsTest.cql
Translation failed due to errors:
[... omitted a bunch of warnings]
Error:[946:17, 946:40] Could not resolve call to operator Slice with signature (list<System.Integer>).
I think for now we just want to put this skip line back, the rest of the changes in this file should be ok.
| const date = Date.parse('2000-06-15'); | ||
| date.successor(Date.Unit.YEAR).should.eql(Date.parse('2001-06-15')); | ||
| date.successor(Date.Unit.MONTH).should.eql(Date.parse('2000-07-15')); | ||
| date.successor(Date.Unit.DAY).should.eql(Date.parse('2000-06-16')); |
There was a problem hiding this comment.
Just an observation, not a request for change at this point - I'm surprised there aren't more leap year tests in this file. I realize date logic is handled by the underlying luxon library, but still
| const val2 = val.clone(); | ||
| val2.value = maxValueForInstance(val2.value); | ||
| return val2; | ||
| // Although the spec says max Quantity has unit '1', it doesn't make sense to change the unit, |
There was a problem hiding this comment.
The comment & approach here make sense but if we're intentionally going against what the spec says, is there a zulip discussion or jira ticket about this as well?
3 participants
This PR replaces #365 by aligning (almost) all interval operators more closely with the exact language and algorithms prescribed in the spec and using the
StartandEndoperators to determine interval boundaries. In addition, interval point types are detected more accurately (usingresultTypewhere possible) to ensure accurate interval representations when both boundaries are null. As a result, handling of closed null and open null boundaries has been greatly improved.Other tangentially related improvements have been made throughout to address additional issues that arose in implementation and testing of the interval operators, especially in regard to arithmetic functions, precision, and data type limits (min/max).
Note:
Submitter:
npm run test:plusto run tests, lint, and prettier)cql4browsers.jsbuilt withnpm run build:browserifyif source changed.Reviewer:
Name: