Leo/sy 779 cesium codebase inspection #648
Conversation
1. an unfinished domain will be skipped and not read in calls to `Next()` 2. when the view is stuck in a discontinuity, the next read cannot read the very next domain.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rc #648 +/- ##
==========================================
+ Coverage 51.65% 51.83% +0.17%
==========================================
Files 1040 1040
Lines 78615 78658 +43
Branches 3249 3249
==========================================
+ Hits 40609 40771 +162
+ Misses 37019 36899 -120
- Partials 987 988 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -138,7 +138,7 @@ func (i *Iterator) Next(ctx context.Context, span telem.TimeSpan) (ok bool) { | |||
|
|
|||
| i.reset(i.view.End.SpanRange(span).BoundBy(i.bounds)) | |||
|
|
|||
| if i.view.IsZero() { | |||
| if i.view.IsZero() || i.view.End.BeforeEq(i.internal.TimeRange().Start) { | |||
There was a problem hiding this comment.
I'd be interested in getting an explanation of why you added this
There was a problem hiding this comment.
If the entire view is before the start of the internal domain, that means there's no data to be read.
Added to address a bug where if the view is put between two domains via Next, it unnecessarily moves on to the next domain and skip the previous one. (domain 1: [10, 20), domain 2: [30, 40), domain 3: [50, 60), say view is at [23, 24), the iterator position is at domain 2 – instead of reading nothing and doing nothing, the iterator actually moved on to domain 3, skipping domain 2 entirely.
| if lengthOfFrame == -1 { | ||
| s := fr.Series[i] | ||
| if s.DataType.Density() == 0 { |
There was a problem hiding this comment.
Can you explain this change?
There was a problem hiding this comment.
We want to check every frame for whether it has the right data type, not just the first.
There was a problem hiding this comment.
We want to check whether every series has the right data type, not just the first
There was a problem hiding this comment.
Shouldn't we be checking this in the unary layer? I added this so that getting the length of the series doesn't panic on a variable length series
There was a problem hiding this comment.
Yes, you're right, we're already checking at the unary level whether the data type is correct. But I still don't understand why we only chekc for the first channel if the data type is unknown. Could you explain what you mean by "variable length series". Isn't getting the length of the frame supposed to panic when the frame is not even?
There was a problem hiding this comment.
Also, just to confirm – at the unary level we check as if int64 and timestamp data type are interchangeable – should that be the case? Should we allow passing a int64 series into a timestamp channel?
fixed bugs: