Add support for nan in RESP3 double

[leibale]

Currenlty RESP3 support 2 "special values": inf and -inf:

,inf<CR><LF>
,-inf<CR><LF>

I think we should add a third one - nan:

,nan<CR><LF>

This is already being used (even though it's not in the spec) in TDIGEST.CDF

[filipecosta90]

raising this issue further. As @leibale stated, if we enable resp3 we break the reply.
going concrete with an example that works with resp2 and that fails with Error: Bad double value as soon we enable resp 3.
What will be the way of handling nan on resp3. Is this something planned for fixing?

127.0.0.1:6379> TDIGEST.BYRANK t1 0
1) "nan"
127.0.0.1:6379> hello 3
1# "server" => "redis"
2# "version" => "255.255.255"
3# "proto" => (integer) 3
4# "id" => (integer) 4
5# "mode" => "standalone"
6# "role" => "master"
7# "modules" => 
   1) 1# "name" => "bf"
      2# "ver" => (integer) 999999
      3# "path" => "/home/fco/redislabs/RedisBloom/bin/linux-x64-release/redisbloom.so"
      4# "args" => (empty array)
127.0.0.1:6379> TDIGEST.BYRANK t1 0
Error: Bad double value

[oranagra]

So in practice it's already like that, and just missing from the spec (and some clients)?
Are we aware of any core command non-module) that exposes that issue?

[itamarhaber]

Yes, it is missing from the spec and already in use.
I'm not aware of NaNs in the core per se, but there is this gem:

127.0.0.1:6379> EVAL "return 0/0" 0
(integer) -9223372036854775808
127.0.0.1:6379> EVAL "return tostring(0/0)" 0
"nan"
127.0.0.1:6379> HELLO 3
1# "server" => "redis"
2# "version" => "7.0.2"
3# "proto" => (integer) 3
4# "id" => (integer) 4
5# "mode" => "standalone"
6# "role" => "master"
7# "modules" => (empty array)
127.0.0.1:6379> EVAL "return 0/0" 0
(integer) -9223372036854775808
127.0.0.1:6379> EVAL "return tostring(0/0)" 0
"nan"

[oranagra]

@itamarhaber what's the verdict here? did we add it to the spec?

[itamarhaber]

Changing the spec is above my paygrade - it requires a full core to do that I assume.

[oranagra]

but we said in practice it's already there (the code uses it), right?
please make a PR, to close this issue, and then ask the core-team to approve.

[filipecosta90]

@oranagra and @itamarhaber I've open a PR to address this on the SPEC in #10 and also a PR to propagate that spec change in redis/hiredis#1133 ( so that after I can propagate the hiredis change to redis )