query: fixing dedup iterator when working on mixed sample types (than…

…os-io#7271) * query: fixing dedup iterator when working on mixed sample types There was a panic in case the dedupiterator worked on two chunks with both Native Histograms and Float (XOR encoded). Co-authored-by: Sebastian Rabenhorst <sebastian.rabenhorst@shopify.com> Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * Adding changelog Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * fixing lint Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * removing comments Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * Fixing repro test case Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * fixing initialization Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * fixing changelog Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * adding header to new file Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * using t.Run Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> * fixing ordering of samples in tests Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> --------- Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com> Co-authored-by: Sebastian Rabenhorst <sebastian.rabenhorst@shopify.com>
jnyi · Jun 3, 2024 · 990fca5 · 990fca5
1 parent e4f0f66
commit 990fca5
Show file tree

Hide file tree

Showing 5 changed files with 163 additions and 85 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 - [#7224](https://github.com/thanos-io/thanos/pull/7224) Query-frontend: Add Redis username to the client configuration.
 - [#7220](https://github.com/thanos-io/thanos/pull/7220) Store Gateway: Fix lazy expanded postings caching partial expanded postings and bug of estimating remove postings with non existent value. Added `PromQLSmith` based fuzz test to improve correctness.
 - [#7244](https://github.com/thanos-io/thanos/pull/7244) Query: Fix Internal Server Error unknown targetHealth: "unknown" when trying to open the targets page.
+- [#7271](https://github.com/thanos-io/thanos/pull/7271) Query: fixing dedup iterator when working on mixed sample types.
 
 ### Added
 

diff --git a/pkg/compact/downsample/downsample_test.go b/pkg/compact/downsample/downsample_test.go
@@ -29,6 +29,7 @@ import (
 
 	"github.com/thanos-io/thanos/pkg/block"
 	"github.com/thanos-io/thanos/pkg/block/metadata"
+	"github.com/thanos-io/thanos/pkg/testutil/testiters"
 )
 
 func TestMain(m *testing.M) {
@@ -1511,26 +1512,26 @@ func TestApplyCounterResetsIteratorHistograms(t *testing.T) {
 
 	histograms := tsdbutil.GenerateTestHistograms(lenChunks * lenChunk)
 
-	var chunks [][]*histogramPair
+	var chunks [][]*testiters.HistogramPair
 	for i := 0; i < lenChunks; i++ {
-		var chunk []*histogramPair
+		var chunk []*testiters.HistogramPair
 		for j := 0; j < lenChunk; j++ {
-			chunk = append(chunk, &histogramPair{t: int64(i*lenChunk+j) * 100, h: histograms[i*lenChunk+j]})
+			chunk = append(chunk, &testiters.HistogramPair{T: int64(i*lenChunk+j) * 100, H: histograms[i*lenChunk+j]})
 		}
 		chunks = append(chunks, chunk)
 	}
 
-	var expected []*histogramPair
+	var expected []*testiters.HistogramPair
 	for i, h := range histograms {
-		expected = append(expected, &histogramPair{t: int64(i * 100), h: h})
+		expected = append(expected, &testiters.HistogramPair{T: int64(i * 100), H: h})
 	}
 
 	for _, tcase := range []struct {
 		name string
 
-		chunks [][]*histogramPair
+		chunks [][]*testiters.HistogramPair
 
-		expected []*histogramPair
+		expected []*testiters.HistogramPair
 	}{
 		{
 			name:     "histogram series",
@@ -1541,21 +1542,21 @@ func TestApplyCounterResetsIteratorHistograms(t *testing.T) {
 		t.Run(tcase.name, func(t *testing.T) {
 			var its []chunkenc.Iterator
 			for _, c := range tcase.chunks {
-				its = append(its, newHistogramIterator(c))
+				its = append(its, testiters.NewHistogramIterator(c))
 			}
 
 			x := NewApplyCounterResetsIterator(its...)
 
-			var res []*histogramPair
+			var res []*testiters.HistogramPair
 			for x.Next() != chunkenc.ValNone {
 				t, h := x.AtHistogram(nil)
-				res = append(res, &histogramPair{t, h})
+				res = append(res, &testiters.HistogramPair{T: t, H: h})
 			}
 			testutil.Ok(t, x.Err())
 			testutil.Equals(t, tcase.expected, res)
 
 			for i := range res[1:] {
-				testutil.Assert(t, res[i+1].t >= res[i].t, "sample time %v is not monotonically increasing. previous sample %v is older", res[i+1], res[i])
+				testutil.Assert(t, res[i+1].T >= res[i].T, "sample time %v is not monotonically increasing. previous sample %v is older", res[i+1], res[i])
 			}
 		})
 	}
@@ -1739,52 +1740,6 @@ func (it *sampleIterator) AtT() int64 {
 	return it.l[it.i].t
 }
 
-type histogramPair struct {
-	t int64
-	h *histogram.Histogram
-}
-
-type histogramIterator struct {
-	l []*histogramPair
-	i int
-}
-
-func newHistogramIterator(l []*histogramPair) *histogramIterator {
-	return &histogramIterator{l: l, i: -1}
-}
-
-func (it *histogramIterator) Err() error {
-	return nil
-}
-
-func (it *histogramIterator) Next() chunkenc.ValueType {
-	if it.i >= len(it.l)-1 {
-		return chunkenc.ValNone
-	}
-	it.i++
-	return chunkenc.ValHistogram
-}
-
-func (it *histogramIterator) Seek(int64) chunkenc.ValueType {
-	panic("unexpected")
-}
-
-func (it *histogramIterator) At() (t int64, v float64) {
-	panic("not implemented")
-}
-
-func (it *histogramIterator) AtHistogram(*histogram.Histogram) (int64, *histogram.Histogram) {
-	return it.l[it.i].t, it.l[it.i].h
-}
-
-func (it *histogramIterator) AtFloatHistogram(*histogram.FloatHistogram) (int64, *histogram.FloatHistogram) {
-	panic("not implemented")
-}
-
-func (it *histogramIterator) AtT() int64 {
-	return it.l[it.i].t
-}
-
 // memBlock is an in-memory block that implements a subset of the tsdb.BlockReader interface
 // to allow tsdb.StreamedBlockWriter to persist the data as a block.
 type memBlock struct {

diff --git a/pkg/dedup/iter.go b/pkg/dedup/iter.go
@@ -296,6 +296,7 @@ func newDedupSeriesIterator(a, b adjustableSeriesIterator) *dedupSeriesIterator
 		b:        b,
 		lastT:    math.MinInt64,
 		lastIter: a,
+		useA:     true,
 		aval:     a.Next(),
 		bval:     b.Next(),
 	}