search

square / metrics

Metrics Query Engine
Apache License 2.0
170 stars 21 forks source link

Compression code issue #260

Closed dgryski closed 8 years ago

dgryski commented 8 years ago

I noticed you have implemented the floating-point compression algorithm from Facebook's Gorilla paper. I also have an implementation which the InfluxDB team discovered an issue in, and I believe it applies to your code as well.

https://github.com/dgryski/go-tsz/issues/4

There was another issue with overflow that I discovered and fixed at the same time: https://github.com/dgryski/go-tsz/commit/918e888e1d33760f5ab18e41cab3857c0037abfe .

Nathan-Fenner commented 8 years ago

Thanks for this bug report. It looks like the first issue is also an issue in our implementation. It's been a low priority since the compression isn't actually used anywhere in our codebase.

I'm not sure how I can tell whether the second issue is also a problem in our codebase.

dgryski commented 8 years ago

The second issue is here

https://github.com/square/metrics/blob/master/util/compress/compress.go#L124

It's possible that both the leading and trailing zeros is 0, which means length is 64, which doesn't fit in 5 bits.

dgryski commented 8 years ago

The PR only closed one of the two issues with the compression. Was that intentional?