From 9aadd547869494371a9499be73eaa15b2668d844 Mon Sep 17 00:00:00 2001
From: beorn7 <beorn@grafana.com>
Date: Tue, 18 Jul 2023 18:17:47 +0200
Subject: [PATCH] histogram: Fix bounds of buckets returned by
 floatBucketIterator

The bounds weren't really used so far, so no actual bug in the code so
far. But it's obviously confusing if the bounds returned by a
floatBucketIterator with a target schema different from the original
schema are wrong.

Signed-off-by: beorn7 <beorn@grafana.com>
---
 model/histogram/float_histogram.go      |  5 +++
 model/histogram/float_histogram_test.go | 47 +++++++++++++++++++++++++
 model/histogram/generic.go              | 14 +++++---
 3 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go
index 782b07df6f..f4a7124a21 100644
--- a/model/histogram/float_histogram.go
+++ b/model/histogram/float_histogram.go
@@ -818,6 +818,11 @@ type floatBucketIterator struct {
 	absoluteStartValue float64 // Never return buckets with an upper bound ≤ this value.
 }
 
+func (i *floatBucketIterator) At() Bucket[float64] {
+	// Need to use i.targetSchema rather than i.baseBucketIterator.schema.
+	return i.baseBucketIterator.at(i.targetSchema)
+}
+
 func (i *floatBucketIterator) Next() bool {
 	if i.spansIdx >= len(i.spans) {
 		return false
diff --git a/model/histogram/float_histogram_test.go b/model/histogram/float_histogram_test.go
index ce749b7101..dd3e30427e 100644
--- a/model/histogram/float_histogram_test.go
+++ b/model/histogram/float_histogram_test.go
@@ -2205,3 +2205,50 @@ func TestAllReverseFloatBucketIterator(t *testing.T) {
 		})
 	}
 }
+
+func TestFloatBucketIteratorTargetSchema(t *testing.T) {
+	h := FloatHistogram{
+		Count:  405,
+		Sum:    1008.4,
+		Schema: 1,
+		PositiveSpans: []Span{
+			{Offset: 0, Length: 4},
+			{Offset: 1, Length: 3},
+			{Offset: 2, Length: 3},
+		},
+		PositiveBuckets: []float64{100, 344, 123, 55, 3, 63, 2, 54, 235, 33},
+		NegativeSpans: []Span{
+			{Offset: 0, Length: 3},
+			{Offset: 7, Length: 4},
+			{Offset: 1, Length: 3},
+		},
+		NegativeBuckets: []float64{10, 34, 1230, 54, 67, 63, 2, 554, 235, 33},
+	}
+	expPositiveBuckets := []Bucket[float64]{
+		{Lower: 0.25, Upper: 1, LowerInclusive: false, UpperInclusive: true, Count: 100, Index: 0},
+		{Lower: 1, Upper: 4, LowerInclusive: false, UpperInclusive: true, Count: 522, Index: 1},
+		{Lower: 4, Upper: 16, LowerInclusive: false, UpperInclusive: true, Count: 68, Index: 2},
+		{Lower: 16, Upper: 64, LowerInclusive: false, UpperInclusive: true, Count: 322, Index: 3},
+	}
+	expNegativeBuckets := []Bucket[float64]{
+		{Lower: -1, Upper: -0.25, LowerInclusive: true, UpperInclusive: false, Count: 10, Index: 0},
+		{Lower: -4, Upper: -1, LowerInclusive: true, UpperInclusive: false, Count: 1264, Index: 1},
+		{Lower: -64, Upper: -16, LowerInclusive: true, UpperInclusive: false, Count: 184, Index: 3},
+		{Lower: -256, Upper: -64, LowerInclusive: true, UpperInclusive: false, Count: 791, Index: 4},
+		{Lower: -1024, Upper: -256, LowerInclusive: true, UpperInclusive: false, Count: 33, Index: 5},
+	}
+
+	it := h.floatBucketIterator(true, 0, -1)
+	for i, b := range expPositiveBuckets {
+		require.True(t, it.Next(), "positive iterator exhausted too early")
+		require.Equal(t, b, it.At(), "bucket %d", i)
+	}
+	require.False(t, it.Next(), "positive iterator not exhausted")
+
+	it = h.floatBucketIterator(false, 0, -1)
+	for i, b := range expNegativeBuckets {
+		require.True(t, it.Next(), "negative iterator exhausted too early")
+		require.Equal(t, b, it.At(), "bucket %d", i)
+	}
+	require.False(t, it.Next(), "negative iterator not exhausted")
+}
diff --git a/model/histogram/generic.go b/model/histogram/generic.go
index e1de5ffb52..dad54cb069 100644
--- a/model/histogram/generic.go
+++ b/model/histogram/generic.go
@@ -102,16 +102,22 @@ type baseBucketIterator[BC BucketCount, IBC InternalBucketCount] struct {
 }
 
 func (b baseBucketIterator[BC, IBC]) At() Bucket[BC] {
+	return b.at(b.schema)
+}
+
+// at is an internal version of the exported At to enable using a different
+// schema.
+func (b baseBucketIterator[BC, IBC]) at(schema int32) Bucket[BC] {
 	bucket := Bucket[BC]{
 		Count: BC(b.currCount),
 		Index: b.currIdx,
 	}
 	if b.positive {
-		bucket.Upper = getBound(b.currIdx, b.schema)
-		bucket.Lower = getBound(b.currIdx-1, b.schema)
+		bucket.Upper = getBound(b.currIdx, schema)
+		bucket.Lower = getBound(b.currIdx-1, schema)
 	} else {
-		bucket.Lower = -getBound(b.currIdx, b.schema)
-		bucket.Upper = -getBound(b.currIdx-1, b.schema)
+		bucket.Lower = -getBound(b.currIdx, schema)
+		bucket.Upper = -getBound(b.currIdx-1, schema)
 	}
 	bucket.LowerInclusive = bucket.Lower < 0
 	bucket.UpperInclusive = bucket.Upper > 0