MergeWith after calling Clear panics
mheffner opened this issue · 2 comments
mheffner commented
Describe what happened:
Running MergeWith() with a sketch that has been recently cleared will panic in the buffered_paginated store:
panic: runtime error: index out of range [8] with length 8 [recovered]
panic: runtime error: index out of range [8] with length 8
goroutine 6 [running]:
testing.tRunner.func1.2({0x79f1c0, 0xc0000223d8})
/go/1.19.4/go/src/testing/testing.go:1396 +0x24e
testing.tRunner.func1()
/go/1.19.4/go/src/testing/testing.go:1399 +0x39f
panic({0x79f1c0, 0xc0000223d8})
/go/1.19.4/go/src/runtime/panic.go:884 +0x212
github.com/DataDog/sketches-go/ddsketch/store.(*BufferedPaginatedStore).page(0xc0000165a0, 0x8000000000000004, 0x2?)
/git/clones/sketches-go/ddsketch/store/buffered_paginated.go:128 +0x5c5
github.com/DataDog/sketches-go/ddsketch/store.(*BufferedPaginatedStore).MergeWith(0xc0000165a0, {0x85abd8?, 0xc000016640?})
/git/clones/sketches-go/ddsketch/store/buffered_paginated.go:383 +0x15b
github.com/DataDog/sketches-go/ddsketch.(*DDSketch).MergeWith(0xc000034d40, 0xc000034d80)
/git/clones/sketches-go/ddsketch/ddsketch.go:300 +0x64
github.com/DataDog/sketches-go/ddsketch.TestMergeAfterClear(0x0?)
/git/clones/sketches-go/ddsketch/ddsketch_test.go:456 +0xf4
testing.tRunner(0xc0000da820, 0x7f3528)
/go/1.19.4/go/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
Describe what you expected:
Expected this would simply skip the merge since the other sketch was cleared.
Steps to reproduce the issue:
I can reproduce with the following test case:
func TestMergeAfterClear(t *testing.T) {
s1, err := NewDefaultDDSketch(0.01)
require.NoError(t, err)
s2, err := NewDefaultDDSketch(0.01)
require.NoError(t, err)
for i := 0; i < 1000; i++ {
err := s2.Add(rand.NormFloat64())
require.NoError(t, err)
}
s2.Clear()
err = s1.MergeWith(s2)
require.NoError(t, err)
}
CharlesMasson commented
Thank you for reporting this bug and providing a failing test case. This should be fixed by #72.
mheffner commented
Thanks for the quick turn around @CharlesMasson !