Repository navigation
Conversation
| }), | ||
| }, | ||
| { | ||
| name: "Collapse empty list", |
There was a problem hiding this comment.
This test name is wrong, technically this is null, an empty list would be {} as ...
We would probably want tests for both though :)
| }), | ||
| }, | ||
| StaticType: &types.List{ElementType: &types.Interval{PointType: types.Integer}}, | ||
| }), |
There was a problem hiding this comment.
Could you add some tests for cases that shouldn't be collapsed, both with closed and open intervals?
| }), | ||
| }, | ||
| { | ||
| name: "Original failing case - sorted order", |
There was a problem hiding this comment.
I believe you already have this case on line 2164 consider removing.
| "Union", | ||
| }, | ||
| NamesExcludes: []string{ | ||
| "TestCollapseNull", |
There was a problem hiding this comment.
just curious, what error were you getting for this test?
| } | ||
| } | ||
|
|
||
| if len(nonNullIntervals) == 0 { |
There was a problem hiding this comment.
You could also return early here if the length is 1.
| } | ||
|
|
||
| // Sort temporal intervals by start time | ||
| sort.Slice(nonNullIntervals, func(i, j int) bool { |
There was a problem hiding this comment.
Can't you use the helper function you added above for this?
| } | ||
|
|
||
| // sortIntervals sorts a list of intervals by their start time | ||
| func sortIntervals(intervals []result.Value, evaluationTimestamp time.Time) ([]result.Value, error) { |
There was a problem hiding this comment.
I like where this function is going but I have a few thoughts.
- I think sortIntervalsByStartTime would be a more apt name here.
- What do you think of using generics here similar to
cql/interpreter/operator_arithmetic.go
Line 321 in 25fd8b5
since sorting for time based values and numerals are such different operations it may make sense to split the two implementations out in this way.
| } | ||
|
|
||
| // Sort numeric intervals by start value | ||
| sort.Slice(nonNullIntervals, func(i, j int) bool { |
There was a problem hiding this comment.
same feedback as the above function i think you can reuse your helper here
| } | ||
| } | ||
|
|
||
| func TestCollapse(t *testing.T) { |
There was a problem hiding this comment.
I think some of the logic for null values like Interval[null, 5] or Interval(null, 5] might not be handled correctly, at least the canMerge functions seem to be marking them all as false for their ability to merge. Mind adding some test cases likecollapse { Inverval[null, 5], Interval[1, 2] } as well as collapse { Inverval(null, 5], Interval[1, 2] } and collapse { Inverval[null, 5], Interval[4, 6] }? I'd feel better about this code knowing those do what I expect :)
|
Since there's been no activity here for a while but this change is mostly there I'm going to conditionally approve it and then manually address the current comments. |
…ore tests - Replace the per-type collapse code with one collapse() that takes a point comparator; numerals share a generic comparator built on applyToValues and compareNumeral. - Rename sortIntervals to sortIntervalsByStartTime and use it everywhere. - Return early for zero or one interval. - Merge when intervals provably overlap or meet, using successor for meets, so Date, DateTime and Time intervals collapse at their precision. A null bound is bounded by the other end of its own interval. - Keep the original bounds of merged intervals instead of rebuilding Dates in UTC; return null for a null list, as the spec says. - Tests: one TestCollapse table, null and empty list cases, intervals that must not collapse (closed and open), null bound cases, duplicate case removed. - DateTimeCollapse2 and TimeCollapse2 now pass; TestCollapseNull stays excluded with the other Interval(null, null) ambiguous match tests.
Sort intervals by start and then end, and sort a coarser DateTime before a finer one it cannot be ordered against. Before, mixed-precision starts left the input order in place, so the same intervals could collapse differently. Add the Interval<Long> overloads, which converted to Decimal before.
f16e737 to
3bd02da
Compare
2 participants
No description provided.