#138 add support for collapse #139
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 :)
2 participants
No description provided.