Fix conversion of numbers in lua args to redis args #13115
Conversation
fpconv_dtoa converts 100000000 (and multiples thereof) to the form 1e9 which breaks the later conversion from string back to number in getLongLongFromObjectOrReply. Switch to using ll2string if the number can be safely represented as a long long. redis#13113
Co-authored-by: debing.sun <debing.sun@redis.com>
|
if it is long long, what abotu create the string robj with long long, instead of converting to str and converting it to long long |
Do you mean like this? Its a little bit awkward because of needing to skip the rest of the loop and I'm not clear on the consequences of skipping the code in the |
|
Would it make sense to add a |
i think it works, and please do a simple benchmark to see if it can bring benefit, thanks. |
I don't know how to do benchmarks within redis and I can't find anything that documents it. Can you point me in the right direction for that? |
Like this |
Excellent, thank you! |
Kind of surprised by this but the version of the PR posted here was faster than the version that I changed to use createStringObjectFromLongLong. For reference that version is available at https://github.com/mdouglass/redis/tree/fix/13113-hincrby-bug-alt. However, both fixes were faster than the original code (about 5.5% and about 2.3%) original codethis PRalternate fix with createStringObjectFromLongLong |
|
@mdouglass thanks, if so we can keep this one. |
|
test in my local
|
There was a problem hiding this comment.
from what i remember from #10587 (@filipecosta90 correct me if i'm wrong), that change didn't conceptually change the output format.
even before that change, it could be that the code would return the scientific notation.
i.e. %g means either %f or %e, which ever is shorter.
maybe i don't understand the root of the problem you're trying to solve.
the behavior of |
|
ohh, only now i notice that this case tries to feed the string into code that only supports integers. |
Yeah, I simplified the lua for the repro. For the record, the original code was trying to do the equivalent of a saturating HINCRBY, so it did something closer to: We had a test that was trying to test the saturation logic and it happened to set |
There was a problem hiding this comment.
Good fix.
Strictly speaking though, the arguments to a Redis command are always strings (RESP bulk). If you pass anything else to redis.call, you're relying on implicit conversion to string, so someone could argue that it's the user's responsibility to convert it to a string in the proper way (rounding, etc.) before doing redis.call.
When using string concatenation .. you get another implicit string conversion in Lua, something a user just needs to be aware of:
127.0.0.1:6379> eval 'return 100000000000000 .. ""' 0
"1e+14"
127.0.0.1:6379> eval 'return 10000000000000 .. ""' 0
"10000000000000"
oranagra
added
the
release-notes
|
Thank you all! |
Since lua_Number is not explicitly an integer or a double, we need to
make an effort
to convert it as an integer when that's possible, since the string could
later be used
in a context that doesn't support scientific notation (e.g. 1e9 instead
of 100000000).
Since fpconv_dtoa converts numbers with the equivalent of `%f` or `%e`,
which ever is shorter,
this would break if we try to pass a long integer number to a command
that takes integer.
we'll get an implicit conversion to string in Lua, and then the parsing
in getLongLongFromObjectOrReply will fail.
```
> eval "redis.call('hincrby', 'key', 'field', '1000000000')" 0
(nil)
> eval "redis.call('hincrby', 'key', 'field', tonumber('1000000000'))" 0
(error) ERR value is not an integer or out of range script: ac99c32e4daf7e300d593085b611de261954a946, on @user_script:1.
```
Switch to using ll2string if the number can be safely represented as a
long long.
The problem was introduced in redis#10587 (Redis 7.2).
closes redis#13113.
---------
Co-authored-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: debing.sun <debing.sun@redis.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 5fdaa53)
Since lua_Number is not explicitly an integer or a double, we need to
make an effort
to convert it as an integer when that's possible, since the string could
later be used
in a context that doesn't support scientific notation (e.g. 1e9 instead
of 100000000).
Since fpconv_dtoa converts numbers with the equivalent of `%f` or `%e`,
which ever is shorter,
this would break if we try to pass a long integer number to a command
that takes integer.
we'll get an implicit conversion to string in Lua, and then the parsing
in getLongLongFromObjectOrReply will fail.
```
> eval "redis.call('hincrby', 'key', 'field', '1000000000')" 0
(nil)
> eval "redis.call('hincrby', 'key', 'field', tonumber('1000000000'))" 0
(error) ERR value is not an integer or out of range script: ac99c32e4daf7e300d593085b611de261954a946, on @user_script:1.
```
Switch to using ll2string if the number can be safely represented as a
long long.
The problem was introduced in #10587 (Redis 7.2).
closes #13113.
---------
Co-authored-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: debing.sun <debing.sun@redis.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 5fdaa53)
Since lua_Number is not explicitly an integer or a double, we need to make an effort
to convert it as an integer when that's possible, since the string could later be used
in a context that doesn't support scientific notation (e.g. 1e9 instead of 100000000).
Since fpconv_dtoa converts numbers with the equivalent of
%for%e, which ever is shorter,this would break if we try to pass a long integer number to a command that takes integer.
we'll get an implicit conversion to string in Lua, and then the parsing in getLongLongFromObjectOrReply will fail.
Switch to using ll2string if the number can be safely represented as a
long long.
The problem was introduced in #10587 (Redis 7.2).
closes #13113.