Skip to content

Conversation

@hdrodz
Copy link

@hdrodz hdrodz commented Apr 22, 2023

Fixes #22. (*TDigest).Quantile() causes an OOB panic when the digest has been provided with values that, due to floating point arithmetic errors, cause the sort.Search call to not find an i such that t.cumulative[i] >= index. (Minimal playground reproduction). This PR fixes this bug by clamping access to processed.Len().

Furthermore, the unit tests as written did not pass on my M2 MacBook Pro: due to differences in how the M2 performs floating point math compared to Intel or AMD CPUs, there are small deltas some 14 digits into the results, so got == tt.expected evaluates to false:

$ go test
--- FAIL: TestTdigest_Quantile (0.00s)
    --- FAIL: TestTdigest_Quantile/uniform_50 (0.00s)
        tdigest_test.go:217: unexpected quantile 0.500000, got 49.99250234584355 want 49.992502345843555
    --- FAIL: TestTdigest_Quantile/uniform_99 (0.00s)
        tdigest_test.go:217: unexpected quantile 0.990000, got 98.98503400959561 want 98.98503400959562
    --- FAIL: TestTdigest_Quantile/uniform_99.9 (0.00s)
        tdigest_test.go:217: unexpected quantile 0.999000, got 99.9010378104362 want 99.90103781043621
FAIL
exit status 1
FAIL    github.com/influxdata/tdigest   1.404s

This PR fixes tests on this hardware by updating to a more recent version of gonum and using its scalar.EqualWithinRel function to compare floats.

@@ -1,9 +1,120 @@
dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yo this go.sum tho

@vroldanbet
Copy link

Hey 👋🏻 We quickly hit this panic when running on ARM-based VMs, rendering the module unusable. Is there a reason this is not being merged?

josephschorr pushed a commit to authzed/spicedb that referenced this pull request Mar 24, 2025
vendors influxdata/tdigest#32 for the purpose of
validating the functionality without the panic. We will likely follow up by replacing the library
as this tdigest does not seem to be maintained anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic: tdigest.go:182, index out of range [40] with length 40

3 participants