DataDog/sketches-go

MergeWith after calling Clear panics

mheffner opened this issue · 2 comments

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)
}

Thank you for reporting this bug and providing a failing test case. This should be fixed by #72.

Thanks for the quick turn around @CharlesMasson !