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 <fiona.y.liao@gmail.com>

tsdb/querier.go

@@ -866,24 +866,32 @@ func (p *populateWithDelChunkSeriesIterator) Next() bool {
 	}
 
 	// Move to the next chunk/deletion iterator.
-	if !p.next(true) {
+	// This is a for loop as if the current p.currDelIter returns no samples
-		return false
+	// (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.currMeta.Chunk != nil {
 		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
+		if p.currMeta.Chunk != nil {
-	// created from the samples in p.currDelIter.
+			// If ChunkOrIterable() returned a non-nil chunk, the samples in
-	return p.populateChunksFromIterable()
+			// 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

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)
+}