alicebob / miniredis

Pure Go Redis server for Go unittests
MIT License
3.05k stars 212 forks source link

fix: zscore return 0 when double value is too small #344

Closed zsh1995 closed 11 months ago

zsh1995 commented 1 year ago

I use miniredis to zadd 0.000000000000000000000000000000000000000000000000000000000000000025339988685347402, then zscore command return a 0, while using real redis server can correctly execute the zadd and zscore command. I found this issuse: https://github.com/alicebob/miniredis/pull/17 already trying to fix this problem, but didn't pass integration test. I read redis's source code https://github.com/redis/redis/blob/unstable/deps/fpconv/fpconv_dtoa.c#L349 , I believe my approach is basically consistent with it.

alicebob commented 1 year ago

Hi! Thanks for the PR. That was already painful to get working back then :( Currently the tests fail with the normal floating point problems...

zsh1995 commented 12 months ago

Thanks Reply! I try this on redis:

127.0.0.1:7079> zadd a_zset 1.2 one
(integer) 0
127.0.0.1:7079> zadd a_zset incr 1.2 one
"2.3999999999999999"
127.0.0.1:7079> zadd a_zset incr 1.2 one
"3.5999999999999996"
127.0.0.1:7079> zadd a_zset incr 1.2 one
"4.7999999999999998"

It seems that if we want to be consistent with redis, we need to adjust the test cases

alicebob commented 12 months ago

On Wed, Sep 20, 2023 at 08:13:32PM -0700, zsh1995 wrote:

Thanks Reply! I try this on redis:

127.0.0.1:7079> zadd a_zset 1.2 one
(integer) 0
127.0.0.1:7079> zadd a_zset incr 1.2 one
"2.3999999999999999"
127.0.0.1:7079> zadd a_zset incr 1.2 one
"3.5999999999999996"
127.0.0.1:7079> zadd a_zset incr 1.2 one
"4.7999999999999998"

It seems that if we want to be consistent with redis, we need to adjust the test cases

Oh, that's interesting. I'll have a proper look next week.

alicebob commented 11 months ago

I'm getting different results, with redis 7.2 on an AMD64 machine:

~/src/miniredis/integration/redis_src$ ./redis-cli 
127.0.0.1:6379> zadd a_zset 1.2 one
(integer) 0
127.0.0.1:6379> zadd a_zset incr 1.2 one
"2.4"
127.0.0.1:6379> zadd a_zset incr 1.2 one
"3.5999999999999996"
127.0.0.1:6379> zadd a_zset incr 1.2 one
"4.8"
127.0.0.1:6379> 
alicebob commented 11 months ago

Redis uses "grisu2" from pdf linked in https://florian.loitsch.com/publications --> https://drive.google.com/file/d/0BwvYOx00EwKmcTZUM0Q4VW1DTHc/view?resourcekey=0-gW-moqRGyWpqFsyhiBI_-w

Go does not do exactly the same thing, but short of implementing grisu2 in Go (sounds fun!), I think this is Good Enough. You were absolutely right in that it should use the "g" option to convert the float (meaning: switch to scientific if needed).

alicebob commented 11 months ago

short of implementing grisu2 in Go

I did just that (#347), but it needs a bit more work still.

alicebob commented 11 months ago

Thanks, merged! I'll merge the grisu pr over this as well, so the formatting in real and mini redis should be identical.