From ce126230e70901a9b139febd723b3a4cc330a88e Mon Sep 17 00:00:00 2001 From: Fiona Liao Date: Wed, 29 Nov 2023 10:24:04 +0000 Subject: [PATCH] Fix chunks iterator bug when tombstone covers a whole chunk (#13209) When no samples are returned in a chunk because all the samples have been deleted, the chunk iterator then stops without iterating through any remaining chunks. Signed-off-by: Fiona Liao --- tsdb/querier.go | 34 ++++++++++------ tsdb/querier_test.go | 95 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 13 deletions(-) diff --git a/tsdb/querier.go b/tsdb/querier.go index a32c7dd96..6584d7da0 100644 --- a/tsdb/querier.go +++ b/tsdb/querier.go @@ -866,24 +866,32 @@ func (p *populateWithDelChunkSeriesIterator) Next() bool { } // Move to the next chunk/deletion iterator. - if !p.next(true) { - return false - } - - if p.currMeta.Chunk != nil { + // This is a for loop as if the current p.currDelIter returns no samples + // (which means a chunk won't be created), there still might be more + // samples/chunks from the rest of p.metas. + for p.next(true) { if p.currDelIter == nil { p.currMetaWithChunk = p.currMeta return true } - // If ChunkOrIterable() returned a non-nil chunk, the samples in - // p.currDelIter will only form one chunk, as the only change - // p.currDelIter might make is deleting some samples. - return p.populateCurrForSingleChunk() - } - // If ChunkOrIterable() returned an iterable, multiple chunks may be - // created from the samples in p.currDelIter. - return p.populateChunksFromIterable() + if p.currMeta.Chunk != nil { + // If ChunkOrIterable() returned a non-nil chunk, the samples in + // p.currDelIter will only form one chunk, as the only change + // p.currDelIter might make is deleting some samples. + if p.populateCurrForSingleChunk() { + return true + } + } else { + // If ChunkOrIterable() returned an iterable, multiple chunks may be + // created from the samples in p.currDelIter. + if p.populateChunksFromIterable() { + return true + } + } + + } + return false } // populateCurrForSingleChunk sets the fields within p.currMetaWithChunk. This diff --git a/tsdb/querier_test.go b/tsdb/querier_test.go index ce46a5c12..6307587f5 100644 --- a/tsdb/querier_test.go +++ b/tsdb/querier_test.go @@ -1038,6 +1038,24 @@ func TestPopulateWithTombSeriesIterators(t *testing.T) { }, expectedMinMaxTimes: []minMaxTimes{{1, 3}, {9, 9}}, }, + { + name: "two chunks with first chunk deleted", + samples: [][]chunks.Sample{ + {sample{1, 2, nil, nil}, sample{2, 3, nil, nil}, sample{3, 5, nil, nil}, sample{6, 1, nil, nil}}, + {sample{7, 89, nil, nil}, sample{9, 8, nil, nil}}, + }, + intervals: tombstones.Intervals{{Mint: 1, Maxt: 6}}, + + expected: []chunks.Sample{ + sample{7, 89, nil, nil}, sample{9, 8, nil, nil}, + }, + expectedChks: []chunks.Meta{ + assureChunkFromSamples(t, []chunks.Sample{ + sample{7, 89, nil, nil}, sample{9, 8, nil, nil}, + }), + }, + expectedMinMaxTimes: []minMaxTimes{{7, 9}}, + }, // Deletion with seek. { name: "two chunks with trimmed first and last samples from edge chunks, seek from middle of first chunk", @@ -3569,3 +3587,80 @@ func TestQueryWithDeletedHistograms(t *testing.T) { }) } } + +func TestQueryWithOneChunkCompletelyDeleted(t *testing.T) { + ctx := context.Background() + db := openTestDB(t, nil, nil) + defer func() { + require.NoError(t, db.Close()) + }() + + db.EnableNativeHistograms() + appender := db.Appender(context.Background()) + + var ( + err error + seriesRef storage.SeriesRef + ) + lbs := labels.FromStrings("__name__", "test") + + // Create an int histogram chunk with samples between 0 - 20 and 30 - 40. + for i := 0; i < 20; i++ { + h := tsdbutil.GenerateTestHistogram(1) + seriesRef, err = appender.AppendHistogram(seriesRef, lbs, int64(i), h, nil) + require.NoError(t, err) + } + for i := 30; i < 40; i++ { + h := tsdbutil.GenerateTestHistogram(1) + seriesRef, err = appender.AppendHistogram(seriesRef, lbs, int64(i), h, nil) + require.NoError(t, err) + } + + // Append some float histograms - float histograms are a different encoding + // type from int histograms so a new chunk is created. + for i := 60; i < 100; i++ { + fh := tsdbutil.GenerateTestFloatHistogram(1) + seriesRef, err = appender.AppendHistogram(seriesRef, lbs, int64(i), nil, fh) + require.NoError(t, err) + } + + err = appender.Commit() + require.NoError(t, err) + + matcher, err := labels.NewMatcher(labels.MatchEqual, "__name__", "test") + require.NoError(t, err) + + // Delete all samples from the int histogram chunk. The deletion intervals + // doesn't cover the entire histogram chunk, but does cover all the samples + // in the chunk. This case was previously not handled properly. + err = db.Delete(ctx, 0, 20, matcher) + require.NoError(t, err) + err = db.Delete(ctx, 30, 40, matcher) + require.NoError(t, err) + + chunkQuerier, err := db.ChunkQuerier(0, 100) + require.NoError(t, err) + + css := chunkQuerier.Select(context.Background(), false, nil, matcher) + + seriesCount := 0 + for css.Next() { + seriesCount++ + series := css.At() + + sampleCount := 0 + it := series.Iterator(nil) + for it.Next() { + chk := it.At() + cit := chk.Chunk.Iterator(nil) + for vt := cit.Next(); vt != chunkenc.ValNone; vt = cit.Next() { + require.Equal(t, vt, chunkenc.ValFloatHistogram, "Only float histograms expected, other sample types should have been deleted.") + sampleCount++ + } + } + require.NoError(t, it.Err()) + require.Equal(t, 40, sampleCount) + } + require.NoError(t, css.Err()) + require.Equal(t, 1, seriesCount) +}