Fixes head chunk iterator direction. #3383
Conversation
For backward queries, since LogQL parser we are using a heapIterator in the headchunk to re-order properly entries. But we also reverse all iterators in the memchunk code, even the headchunk which causes reversal of already reversed entries. This PR skips reversal of the headchunk. Fixes grafana#3345 Fixes grafana#3208 This has for side effects: - when using replication you would not dedupe data properly anymore, since the data is not correctly ordered accross batches. - when using limit it would miss entries. Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
|
Not fully done there's more to it, on it. |
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
|
we're good to go |
There was a problem hiding this comment.
Very nice catch. Some questions then LGTM. I think instead of NewEntryReversedIter, we should expose a declarative option like NewEntryDirectionIter(logproto.Direction) to make this easier to reason about and less likely for us to expose bugs when i.e. extending/refactoring the code. EntryIterator would probably need to support a Direction() logproto.Direction method, but that's easy. I think that approach would make this code much easier to reason about. What do you think?
|
|
||
| process := func(e entry) { | ||
| // apply time filtering | ||
| if e.t < mint || e.t >= maxt { |
There was a problem hiding this comment.
Does this need to account for direction in order to switch inclusivity from [start,end) to (start, end] when switching directions?
There was a problem hiding this comment.
This is a good question, I don't know if we have this really clearly identified somewhere. I think my opinion would be that the query results would be the same regardless of direction, inclusive of start time exclusive of end.
I need to look closer at this code to see if this is doing what I would expect.
There was a problem hiding this comment.
This looks correctly implemented to me based on the assumption that start/end time inclusivity/exclusivity are not based on the requested query direction.
| @@ -867,6 +891,7 @@ func (hb *headBlock) sampleIterator(ctx context.Context, mint, maxt int64, extra | |||
| } | |||
| seriesRes := make([]logproto.Series, 0, len(series)) | |||
| for _, s := range series { | |||
| // todo(ctovena) not sure we need this sort. | |||
There was a problem hiding this comment.
I think we can remove this - they're added in order.
For backward queries, since LogQL parser we are using a heapIterator in the headchunk to re-order properly entries.
But we also reverse all iterators in the memchunk code, even the headchunk which causes reversal of already reversed entries.
This PR skips reversal of the headchunk.
Fixes #3345
Fixes #3208
This has for side effects:
Signed-off-by: Cyril Tovena cyril.tovena@gmail.com