Lua to Redis integer conversion loses precision

[ssimeonov]

Lua has a single number type that follows the IEEE 754 binary64 specification, which allows for 52 bits of precision. This makes the continuous range of positive integers that can be represented without loss of precision [1, (2^53) - 1].

When converting numbers from Lua to Redis, Redis uses default stringification which outputs in scientific notation with limited precision. This has the effect of losing precision for large integers, violating the Redis promise in the scripting documentation:

This conversion between data types is designed in a way that if a Redis type is converted into a Lua type, and then the result is converted back into a Redis type, the result is the same as of the initial value.

Because Redis has to stringify numbers in decimal notation, some precision loss for non-integer floating point numbers is to be expected. (The scripting documentation should probably be updated to point this out.) However, losing precision for integers that can be represented exactly in IEEE 754 is a bug that can have significant impact and can be very difficult to track down. The larger the integers, the larger the precision loss.

To reproduce, put the following in an Rspec suite:

it 'demonstrates Lua to Redis conversion precision loss' do
  redis = Redis.new

  val = 1.0 * ((2**53) - 1)

  lua = <<-EOS
      local value = (2^53) - 1
      local hashData = {
              "defaultPrecision",
              value,
              "extendedPrecision",
              string.format("%.0f", value)
          }
      redis.call("HMSET", KEYS[1], unpack(hashData))
      return redis.call("HGETALL", KEYS[1])
  EOS

  result = redis.eval(lua, keys: %w("test:key"))

  result[2].should == 'extendedPrecision'
  result[3].to_f.should == val

  result[0].should == 'defaultPrecision'
  result[1].to_f.should == val
end

This will generate the following HMSET command:

1369104793.981944 [0 lua] "HMSET" "test:key" "defaultPrecision" "9.007199254741e+15" "extendedPrecision" "9007199254740991"

It will fail with the following message:

     Failure/Error: result[1].to_f.should == val
       expected: 9007199254740991.0
            got: 9007199254741000.0 (using ==)

The suggested fix is to check whether a number is an integer and, in that case, stringify using full precision as opposed to scientific notation.

[josiahcarlson]

Lua's numbers are IEE 754 floating point doubles. Which means only exact precision for integers up to +/- 2**53. This is expected behavior.

[ssimeonov]

@josiahcarlson I am confused. You and I are saying the same thing yet you think this is not a bug?

If you read the spec, it shows loss of precision for 2^53 - 1 (within the range that IEEE 754 specifies) by default and it also shows how the loss of precision can be avoided by actually writing out all the digits of the number represented in Lua.

Further, there are many more integers outside this range which can be represented precisely (but not in a continuous range) in IEEE 754 which will not be correctly passed from Lua to Redis.

[josiahcarlson]

I misread late at night. You are right, the format specifier should be updated. For some reason, I thought that maybe you were expecting integers outside the +/- 2**53 range to be exactly represented in Lua.

My mistake, you're right :)

[kryogenic]

I have this issue too.
Storing a very small or very large floating point number (ex. 1.1710299640683E-305) as a score in a redis zset results in a loss of precision when the float is converted to a string.

[antirez]

Hello, checking this issue right now, report ASAP.

[mattsta]

The default lua number-to-string conversion routine called by lua_tolstring() is using %.14g which is too short for 9007199254740991.

Looks like we need to modify the conversion from a one-line lua_tolstring() to do:

if (lua_isnumber(lua, -1))
  custom format a string from double returned by lua_tonumber(lua, -1)
else
   use lua_tolstring()

[antirez]

Exactly Matt, that's my fix:

diff --git a/src/scripting.c b/src/scripting.c
index 4b485d9..2a7f1b9 100644
--- a/src/scripting.c
+++ b/src/scripting.c
@@ -233,12 +233,22 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error
         char *obj_s;
         size_t obj_len;

-        obj_s = (char*)lua_tolstring(lua,j+1,&obj_len);
-        if (obj_s == NULL) break; /* Not a string. */
+        if (lua_isnumber(lua,j+1)) {
+            /* We can't use lua_tolstring() for number -> string conversion
+             * since Lua uses a format specifier that loses precision. */
+            char dbuf[64];
+            lua_Number num = lua_tonumber(lua,j+1);
+
+            obj_len = snprintf(dbuf,sizeof(dbuf),"%.17g",(double)num);
+            obj_s = dbuf;
+        } else {
+            obj_s = (char*)lua_tolstring(lua,j+1,&obj_len);
+            if (obj_s == NULL) break; /* Not a string. */
+        }

         /* Try to use a cached object. */
-        if (j < LUA_CMD_OBJCACHE_SIZE && cached_objects[j] &&
-            cached_objects_len[j] >= obj_len)
+        if (j < LUA_CMD_OBJCACHE_SIZE && cached_objects[j] &&
+            cached_objects_len[j] >= obj_len)
         {
             char *s = cached_objects[j]->ptr;
             struct sdshdr *sh = (void*)(s-(sizeof(struct sdshdr)));

Do you people like it? The OP's test passes now.

[mattsta]

Looks good! Though, it may be unsafe to keep a reference to dbuf[64] in obj_s after we leave the defining scope.

[antirez]

Thanks @mattsta, yep, the reference will no longer be valid, but exactly like with lua_tostring that is subject to the Lua GC. The code path will use the value only in the context of the code that follows, that never calls Lua nor rewrites the dbuf buffer.

[mattsta]

Oh, I meant we're saving the pointer to dbuf after it goes out of scope. :)

[antirez]

@mattsta oh, right! That's bad. Fixing, thank you. I'm not 100% sure of ANSI C guarantees or not that a stack allocated thing is in scope as long as the function did not returned. Should be only a matter of visibility AFAIK but I'm not 100% sure so better to fix.

[antirez]

Can't find a definitive answer after 5 minutes of Google, and anyway, simpler obvious code is better, so changed the scope of dbuf :-) Closing the issue, thank you @ssimeonov good bug report about a non obvious issue.

[ssimeonov]

@antirez @mattsta with the scope change the fix looks great. Yes, this was a nasty one to track down in some complex Lua scripts. :)

[cyberzx]

Hi.
Today I updated redis to latest version on my desktop and found some weird bug. I cant use 64-bit integer from lua scripts even if they represents as strings. I always get double as result that can't converted back to integer.

For exemple:

127.0.0.1:6379> eval 'return redis.call("set", "key", "12039611435714932082")' 0
OK
127.0.0.1:6379> get key
"1.2039611435714933e+19"
127.0.0.1:6379>

127.0.0.1:6379> eval 'return redis.call("set", "key", "12039611435714932092")' 0
OK
127.0.0.1:6379> get key
"1.2039611435714933e+19"

notice that different integer values have same float representation

[josiahcarlson]

Can confirm bug using Redis 2.8.10 . This should be a test.

[mattsta]

Lua uses 64-bit doubles for all numbers, so, sadly, we can't represent an exact 64-bit value in Lua.

Even in a Lua REPL:

> f = 12039611435714932082                                                                                                   
> =string.format("%f", f)
12039611435714932736.000000

Lua can only operate on exact integers less than about 2^52, so for Lua<->Redis interop, there's not much we can do here. The next version of Lua (5.3) has an exactly 64-bit Integer type, but it's still undergoing development.

[josiahcarlson]

@mattsta You don't understand. There is no passing of a number, there is a passing of a string to the Redis call.

[mattsta]

Ah, I see. Reading comprehension fail on my part.

This bug was introduced by the fix to the main issue of this thread: 768994b

You can see we first try to convert every argument to a Lua number, which, sadly, a string full of numbers is the same as a number to Lua.

I'm not sure we can have the previous fix and this fix together without modifying how Lua handles formatting numbers.

[cyberzx]

I think that changing condition

if (lua_isnumber(lua,j+1))

to

if (lua_isnumber(lua, j+1) && !lua_isstring(lua, j+1))

would help

[mattsta]

Before !lua_isstring() (i.e. 2.9.10 released version)

127.0.0.1:6379> eval 'return redis.call("set", "key", "12039611435714932082")' 0
OK
127.0.0.1:6379> get key
"1.2039611435714933e+19"
127.0.0.1:6379> eval 'return redis.call("set", "key", 2^53-1)' 0
OK
127.0.0.1:6379> get key
"9007199254740991"

So, it currently works for the originally problem in this thread, but fails for the regression.

After adding !lua_isstring()

127.0.0.1:6379> eval 'return redis.call("set", "key", "12039611435714932082")' 0
OK
127.0.0.1:6379> get key
"12039611435714932082"
127.0.0.1:6379> eval 'return redis.call("set", "key", 2^53-1)' 0
OK
127.0.0.1:6379> get key
"9.007199254741e+15"

Now it works for the regression but fails for the original problem in this thread.

Some Lua definitions:

int lua_isnumber (lua_State *L, int index): Returns 1 if the value at the given acceptable index is a number or a string convertible to a number, and 0 otherwise.

int lua_isstring (lua_State *L, int index): Returns 1 if the value at the given acceptable index is a string or a number (which is always convertible to a string), and 0 otherwise.

The problem: When you call lua_tolstring() on a Lua value to get a string value, if the string contains a number, Lua converts the string to a double for internal usage. Then, to add insult to casting, it prints the double in less than full precision: luaconf.h:#define LUA_NUMBER_FMT "%.14g"

[josiahcarlson]

bool real_isstring(lua_State *L, int index) {
    int t = lua_type(L, index);
    return t == LUA_TSTRING;
}

[mattsta]

Using:

if (lua_isnumber(lua, j+1) && !luaReallyIsString(lua, j+1)) {

127.0.0.1:6379> eval 'return redis.call("set", "key", "12039611435714932082")' 0
OK
127.0.0.1:6379> get key
"12039611435714932082"
127.0.0.1:6379> eval 'return redis.call("set", "key", 2^53-1)' 0
OK
127.0.0.1:6379> get key
"9007199254740991"
127.0.0.1:6379> 

Looks good and make test still passes!

I'll add a test for the regression and throw a PR up.

Thanks for making the solution, @josiahcarlson; and thanks everybody else for doing a few rounds of "why is this broken!"

[josiahcarlson]

It is likely a bit faster to do the luaReallyIsString() call before the isnumber call.

[mattsta]

Good call. It'll save potential unnecessary number conversion work we may throw away.

[antirez]

Thanks people, merging tomorrow morning and releasing 2.8.11 at the same time.

[antirez]

I finally read the thread, isn't the following a more straightforward fix?

diff --git a/src/scripting.c b/src/scripting.c
index 2796477..c968adb 100644
--- a/src/scripting.c
+++ b/src/scripting.c
@@ -234,7 +234,7 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error)
         size_t obj_len;
         char dbuf[64];

-        if (lua_isnumber(lua,j+1)) {
+        if (lua_type(lua,j+1) == LUA_TNUMBER) {
             /* We can't use lua_tolstring() for number -> string conversion
              * since Lua uses a format specifier that loses precision. */
             lua_Number num = lua_tonumber(lua,j+1);

[mattsta]

Yeah, that looks even better. It removes any possibility of converting strings into numbers, so we don't need to check for string types anymore.

[antirez]

Exactly, I don't think there are any non-string-type -> number conversion rules in Lua, so probably the code is functionally equivalent, however the smaller patch is more clear in the intent, that is, enter the branch only if it's really a number.

Matt, I'm using the test from your PR, what is the best way to change stuff like that? I may:

• Apply your PR, commit this change to simplify it.
• Apply your PR locally, amend it adding my comment in the commit message where I say I modified it a bit.
• Apply my change, apply your PR, resolve conflicts by just taking the test part.
• Any other idea.

Please teach some Gittetiquette ;-)

[antirez]

Ok just read Matt is going to bed (the power of Twitter), I'll handle this. Thanks!

[antirez]

Fix applied.

Rant: Lua API is not perfect in this regard. The lack of a true integer type distinct from float was already a pain point, shared with Javascript, but given the "we break old stuff" philosophy of Lua (that I like) it's shame it was not fixed at some point much earlier. However even with this type system, the API could be much better, without explicit conversions if not via specially named APIs like lua_is_convertible_to_number or alike, but really, an API called is_number should not do automatic coercion.

[ignacio]

Indeed, automatic coercion should go away, although, to be fair, the manual indicates it. There is some talk of taking the upcoming 5.3 release as an oportunity to remove it (the manual says it may be removed ).
Regarding, the lack of an integer type, Lua 5.3 will finally have 64 bit integers, but script authors will have to watch out for some surprises :)

Lua 5.3.0 (work2)  Copyright (C) 1994-2014 Lua.org, PUC-Rio
> 2+"3"
5.0

[bybera]

Hello, i am working on windows machine. And redis version 3.2.100 for windows latest pre release version. And I have same problem. Can you help me?

[szmcdull]

The latest Lua can handle 64 bit integer. Like it can detect a number is of integer or float. So tonumber(999999999999999999) return the exact number (18 of 9s)