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 782b07df6..f4a7124a2 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 ce749b710..dd3e30427 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 e1de5ffb5..dad54cb06 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