From e7592fe35391ca488671e8997ddf285c179afbc9 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 4 Jan 2022 12:18:43 +0100 Subject: [PATCH] sparsehistogram: Address two TODOs Signed-off-by: beorn7 --- model/histogram/float_histogram.go | 7 +++++-- model/histogram/float_histogram_test.go | 2 -- promql/engine_test.go | 2 +- storage/interface.go | 6 +++++- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index a3bfc6bb7..b941bfd08 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -470,7 +470,10 @@ type FloatBucket struct { Lower, Upper float64 LowerInclusive, UpperInclusive bool Count float64 - Index int32 // Index within schema. To easily compare buckets that share the same schema. + + // Index within schema. To easily compare buckets that share the same + // schema and sign (positive or negative). Irrelevant for the zero bucket. + Index int32 } // String returns a string representation of a FloatBucket, using the usual @@ -686,7 +689,7 @@ func (r *allFloatBucketIterator) Next() bool { LowerInclusive: true, UpperInclusive: true, Count: r.h.ZeroCount, - Index: math.MinInt32, // TODO(codesome): What is the index for this? + // Index is irrelevant for the zero bucket. } return true } diff --git a/model/histogram/float_histogram_test.go b/model/histogram/float_histogram_test.go index 5c8bce248..6d43ea429 100644 --- a/model/histogram/float_histogram_test.go +++ b/model/histogram/float_histogram_test.go @@ -15,7 +15,6 @@ package histogram import ( "fmt" - "math" "testing" "github.com/stretchr/testify/require" @@ -973,7 +972,6 @@ func TestAllFloatBucketIterator(t *testing.T) { LowerInclusive: true, UpperInclusive: true, Count: c.h.ZeroCount, - Index: math.MinInt32, }) } if c.includePos { diff --git a/promql/engine_test.go b/promql/engine_test.go index 3e07b4f4c..02baca9b4 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -3002,8 +3002,8 @@ func TestSparseHistogram_Sum_AddOperator(t *testing.T) { ts := int64(i+1) * int64(10*time.Minute/time.Millisecond) app := test.Storage().Appender(context.TODO()) for idx, h := range c.histograms { - // TODO(codesome): Without h.Copy(), it was working weird. TSDB is keeping reference of last histogram. AppendHistogram should not take pointers! lbls := labels.FromStrings("__name__", seriesName, "idx", fmt.Sprintf("%d", idx)) + // Since we mutate h later, we need to create a copy here. _, err = app.AppendHistogram(0, lbls, ts, h.Copy()) } require.NoError(t, err) diff --git a/storage/interface.go b/storage/interface.go index ec47edbf5..7468df56a 100644 --- a/storage/interface.go +++ b/storage/interface.go @@ -234,7 +234,11 @@ type HistogramAppender interface { // histograms in the same or later transactions. Returned reference // numbers are ephemeral and may be rejected in calls to Append() at any // point. Adding the sample via Append() returns a new reference number. - // If the reference is 0 it must not be used for caching. + // If the reference is 0, it must not be used for caching. + // + // For efficiency reasons, the histogram is passed as a + // pointer. AppendHistogram won't mutate the histogram, but in turn + // depends on the caller to not mutate it either. AppendHistogram(ref SeriesRef, l labels.Labels, t int64, h *histogram.Histogram) (SeriesRef, error) }