microsoftarchive / redis

Redis is an in-memory database that persists on disk. The data model is key-value, but many different kind of values are supported: Strings, Lists, Sets, Sorted Sets, Hashes
http://redis.io
Other
20.81k stars 5.37k forks source link

Infinities parsing is inconsistent with Linux build #455

Closed id-ilych closed 8 years ago

id-ilych commented 8 years ago

I just tested zcount behaviour with redis-cli on Windows using 3.0.501 release

127.0.0.1:6379> zadd foo 2 a (integer) 1 127.0.0.1:6379> zadd foo 3 b (integer) 1 127.0.0.1:6379> zadd foo 4 c (integer) 1 127.0.0.1:6379> zcount foo 2 3 (integer) 2 127.0.0.1:6379> zcount foo 2 +Infinity (error) ERR min or max is not a float 127.0.0.1:6379> zcount foo -Infinity +Infinity (error) ERR min or max is not a float 127.0.0.1:6379> zcount foo -Inf +Inf (integer) 3

And on Linux using 2.8.4 (I don't have Linux machine so used Cloud9 and they have that version installed):

127.0.0.1:6379> zadd foo 2 a (integer) 1 127.0.0.1:6379> zadd foo 3 b (integer) 1 127.0.0.1:6379> zadd foo 4 c (integer) 0 127.0.0.1:6379> zcount foo 2 3 (integer) 2 127.0.0.1:6379> zcount foo 2 +Infinity (integer) 3 127.0.0.1:6379> zcount foo -Infinity +Infinity (integer) 3 127.0.0.1:6379> zcount foo -Inf +Inf (integer) 3

So I see two options:

  1. "-Infinity" and "+Infinity" are correct values to use which implies that Windows build has a bug.
  2. Behaviour is undefined/unspecified for "-Infinity"/"+Infinity" and can change in future so these values should not be used.

Note: "-Infinity" and "+Infinity" are what Java's String.valueOf(Double.NEGATIVE_INFINITY) and String.valueOf(Double.POSITIVE_INFINITY) return.

enricogior commented 8 years ago

Hi @id-ilych thank you for reporting the issue. Currently Windows supports +/-Inf and +/-Infinite. We will add support for +/-Infinity. Thanks.

id-ilych commented 8 years ago

@enricogior I tested "Infinite" variant and results are opposite: it works on Windows but doesn't work on Linux...

enricogior commented 8 years ago

@id-ilych yes, I'm not sure where Infinite comes from on the Windows platform, it might have been an oversight when Redis was first ported to Windows and Infinity was changed to Infinite by mistake.

id-ilych commented 8 years ago

@enricogior the funny thing is that strtod should handle both ( http://en.cppreference.com/w/cpp/string/byte/strtod)

Test using g++ (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4:

#include <iostream>
#include <cstdlib>

void test(const char *str) {
    char *p = nullptr;
    double val = strtod(str, &p);
    auto consumed = (p - str);
    std::cout << str << " " << val << " " << consumed << std::endl;
}

int main() {
    test("Infinity");
    test("Infinite");
    test("InfinitU");
    test("Inf");
}

Outputs:

Infinity inf 8
Infinite inf 3
InfinitU inf 3
Inf inf 3

At glance I found that strtod is used in zslParseRange to parse zcount arguments. Can't find special handling for "infinite". Can you point me to the place?

enricogior commented 8 years ago

@id-ilych Infinite inf 3 shows that strtod has parsed/consumed only the first 3 chars, therefore it makes sense that the command parser used by Redis on Linux produces an error trying to parse the following chars.

id-ilych commented 8 years ago

@enricogior You're right. But it doesn't explain how 'infinite' works on Windows :(

enricogior commented 8 years ago

@id-ilych on Windows there is a custom parsing function wstrtod that has support for Inf and Infinite but not Infinity.

id-ilych commented 8 years ago

@enricogior Thank you. I overlooked that #define strtod when looking into code. Though it comes with rather weird comment on strtod being unable to handle NAN and INF which conflicts with http://en.cppreference.com/w/cpp/string/byte/strtod

Anyway thank you for explanation and hope you'll fix this soon :+1:

enricogior commented 8 years ago

@id-ilych the fix has been committed to the repo, it will be included in the next release. Thank you.

enricogior commented 8 years ago

Hi @id-ilych the 3.0.502 release contains the fix for this issue. Thank you.

enricogior commented 8 years ago

@id-ilych update: the latest release is 3.0.503. Sorry about the double release in one day. Thank you.