From ec823d9daf24ed2a9c3d9176759be78b6e0e1c52 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Mon, 9 Oct 2023 16:09:46 +0300 Subject: [PATCH] Update comments, bitwise comparison of float buckets Signed-off-by: Linas Medziunas --- model/histogram/float_histogram.go | 19 ++++++++++++++++--- model/histogram/float_histogram_test.go | 24 +++++++++++++++++++----- model/histogram/generic.go | 12 ------------ model/histogram/histogram.go | 9 ++++++--- 4 files changed, 41 insertions(+), 23 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index f1cb6e5a9..e6e6ec684 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -307,7 +307,8 @@ func (h *FloatHistogram) Sub(other *FloatHistogram) *FloatHistogram { // Exact match is when there are no new buckets (even empty) and no missing buckets, // and all the bucket values match. Spans can have different empty length spans in between, // but they must represent the same bucket layout to match. -// Non-metadata float fields (Sum, Count, ZeroCount) are compared as binary (using math.Float64bits). +// Sum, Count, and ZeroCount are compared based on their bit patterns because this method +// is about data equality rather than mathematical equality. func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { if h2 == nil { return false @@ -327,10 +328,10 @@ func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { return false } - if !bucketsMatch(h.PositiveBuckets, h2.PositiveBuckets) { + if !floatBucketsMatch(h.PositiveBuckets, h2.PositiveBuckets) { return false } - if !bucketsMatch(h.NegativeBuckets, h2.NegativeBuckets) { + if !floatBucketsMatch(h.NegativeBuckets, h2.NegativeBuckets) { return false } @@ -1105,3 +1106,15 @@ func addBuckets( return spansA, bucketsA } + +func floatBucketsMatch(b1, b2 []float64) bool { + if len(b1) != len(b2) { + return false + } + for i, b := range b1 { + if math.Float64bits(b) != math.Float64bits(b2[i]) { + return false + } + } + return true +} diff --git a/model/histogram/float_histogram_test.go b/model/histogram/float_histogram_test.go index 61e8fc515..ae8ba3ea2 100644 --- a/model/histogram/float_histogram_test.go +++ b/model/histogram/float_histogram_test.go @@ -2296,11 +2296,13 @@ func TestFloatBucketIteratorTargetSchema(t *testing.T) { // cannot be covered by TestHistogramEquals. func TestFloatHistogramEquals(t *testing.T) { h1 := FloatHistogram{ - Schema: 3, - Count: 2.2, - Sum: 9.7, - ZeroThreshold: 0.1, - ZeroCount: 1.1, + Schema: 3, + Count: 2.2, + Sum: 9.7, + ZeroThreshold: 0.1, + ZeroCount: 1.1, + PositiveBuckets: []float64{3}, + NegativeBuckets: []float64{4}, } equals := func(h1, h2 FloatHistogram) { @@ -2326,4 +2328,16 @@ func TestFloatHistogramEquals(t *testing.T) { hZeroCountNaN.ZeroCount = math.NaN() notEquals(h1, *hZeroCountNaN) equals(*hZeroCountNaN, *hZeroCountNaN) + + // Positive bucket value is NaN. + hPosBucketNaN := h1.Copy() + hPosBucketNaN.PositiveBuckets[0] = math.NaN() + notEquals(h1, *hPosBucketNaN) + equals(*hPosBucketNaN, *hPosBucketNaN) + + // Negative bucket value is NaN. + hNegBucketNaN := h1.Copy() + hNegBucketNaN.NegativeBuckets[0] = math.NaN() + notEquals(h1, *hNegBucketNaN) + equals(*hNegBucketNaN, *hNegBucketNaN) } diff --git a/model/histogram/generic.go b/model/histogram/generic.go index dad54cb06..48fb906ce 100644 --- a/model/histogram/generic.go +++ b/model/histogram/generic.go @@ -333,18 +333,6 @@ func compactBuckets[IBC InternalBucketCount](buckets []IBC, spans []Span, maxEmp return buckets, spans } -func bucketsMatch[IBC InternalBucketCount](b1, b2 []IBC) bool { - if len(b1) != len(b2) { - return false - } - for i, b := range b1 { - if b != b2[i] { - return false - } - } - return true -} - func getBound(idx, schema int32) float64 { // Here a bit of context about the behavior for the last bucket counting // regular numbers (called simply "last bucket" below) and the bucket diff --git a/model/histogram/histogram.go b/model/histogram/histogram.go index 88883472d..1a7a2de7f 100644 --- a/model/histogram/histogram.go +++ b/model/histogram/histogram.go @@ -17,6 +17,8 @@ import ( "fmt" "math" "strings" + + "golang.org/x/exp/slices" ) // CounterResetHint contains the known information about a counter reset, @@ -172,7 +174,8 @@ func (h *Histogram) CumulativeBucketIterator() BucketIterator[uint64] { // Exact match is when there are no new buckets (even empty) and no missing buckets, // and all the bucket values match. Spans can have different empty length spans in between, // but they must represent the same bucket layout to match. -// Non-metadata float field (Sum) is compared as binary (using math.Float64bits). +// Sum is compared based on its bit pattern because this method +// is about data equality rather than mathematical equality. func (h *Histogram) Equals(h2 *Histogram) bool { if h2 == nil { return false @@ -191,10 +194,10 @@ func (h *Histogram) Equals(h2 *Histogram) bool { return false } - if !bucketsMatch(h.PositiveBuckets, h2.PositiveBuckets) { + if !slices.Equal(h.PositiveBuckets, h2.PositiveBuckets) { return false } - if !bucketsMatch(h.NegativeBuckets, h2.NegativeBuckets) { + if !slices.Equal(h.NegativeBuckets, h2.NegativeBuckets) { return false }