Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zAdd does not set the correct score for values 1000 & above #113

Closed
NiroSugir opened this issue Dec 26, 2011 · 15 comments
Closed

zAdd does not set the correct score for values 1000 & above #113

NiroSugir opened this issue Dec 26, 2011 · 15 comments

Comments

@NiroSugir
Copy link

Hi,
I've been using your API for Redis for a few days now and it's the best by far. It leaves a tiny footprint and has all the functions I need. Keep up the great work.

Now to the bug I came across. When creating a Sorted Set, the score is not properly set when using zAdd. If the score is 1000 or above, the score changes to the first digit only.

PHP Script:
$redis->zAdd('anothertest',1000,'work');

Redis-Cli Monitor:
1324885452.287708 "ZADD" "anothertest" "1" "work"

I've tested it with smaller values (including decimal values) and it's working just fine. Casting the value as a string has the same effect. The score range I was trying to set was the microtime. Redis seems to be able to work with values that high, so I'm pretty sure there's a minor bug somewhere in the API.

If you need more info, let me know.

Testing Machine:
64bit Centos 6.0

@nicolasff
Copy link
Member

Thank, I'll have a look today.

@nicolasff
Copy link
Member

Hello,

I have just tested this without any issue. Do think the problem might be with the return value of microtime()? “By default, microtime() returns a string in the form "msec sec", where sec is the current time measured in the number of seconds since the Unix epoch (0:00:00 January 1, 1970 GMT), and msec is the number of microseconds that have elapsed since sec expressed in seconds.” If you want a floating point value, use microtime(true).

$ cat test.php 
<?php

$redis = new Redis;
$redis->connect('127.0.0.1', 6379);

$redis->del('test');

$t = microtime(true);
$redis->zadd('test', $t, 'x');

var_dump($t, $redis->zrange('test', 0, -1, true));
?>


$ php test.php 
float(1324923009.7921)
array(1) {
  ["x"]=>
  string(17) "1324923009.792141"
}


(redis-cli)

1324923009.792088 "DEL" "test"
1324923009.792215 "ZADD" "test" "1324923009.79214096" "x"
1324923009.792290 "ZRANGE" "test" "0" "-1" "WITHSCORES"

Would you have some example code that fails to send the right command? I also tried 1000, and it was sent as expected.

@NiroSugir
Copy link
Author

Here's the code that I used to attempt this. I was trying to port the login times from MySQL to Redis, so I was testing at each step. Still getting the same issue. I'm using Redis Server 2.4.5 & version 2.1.3 of phpredis on a 64 bit machine. By the looks of it, for some reason it's the API that's sending the wrong value since the Monitor picks up only the first digit. Let me know if you want me to do any further testing.

$redis = new Redis(); $redis->connect('127.0.0.1','6379'); $return = array(); $return[]=$redis->zAdd('login_time',microtime(true),'Niro'); $return[]=$redis->zAdd('login_time',microtime(true),'Another'); $return[]=$redis->zRange('login_time',0,-1,true); echo "<pre>"; var_dump($return); echo "</pre><br>";

Output on web:

  [0]=>
  int(1)
  [1]=>
  int(1)
  [2]=>
  array(2) {
    ["Another"]=>
    string(1) "1"
    ["Niro"]=>
    string(1) "1"
  }
}

Redis-Cli Monitor:

1324924307.436061 "ZADD" "login_time" "1" "Another"
1324924307.436270 "ZRANGE" "login_time" "0" "-1" "WITHSCORES"```

@nicolasff
Copy link
Member

Could you please try the latest version of phpredis? This is the output from your code:

array(3) {
  [0]=>
  int(1)
  [1]=>
  int(1)
  [2]=>
  array(2) {
    ["Niro"]=>
    string(18) "1324925309.8650861"
    ["Another"]=>
    string(18) "1324925309.8652489"
  }
}

redis-cli monitor:

1324925366.509590 "monitor"
1324925367.841260 "ZADD" "login_time" "1324925367.84114599" "Niro"
1324925367.841368 "ZADD" "login_time" "1324925367.84132099" "Another"
1324925367.841457 "ZRANGE" "login_time" "0" "-1" "WITHSCORES"

@NiroSugir
Copy link
Author

I just ran your code, and I'm getting the same wrong score.

float(1324925446.3172)
array(1) {
  ["x"]=>
  string(1) "1"
}

@NiroSugir
Copy link
Author

I have found the problem and it's weird. I was using the 3rd Release Candidate of PHP 5.4. Downgrading to version 5.3.3 fixed the problem. Not quite sure why that would cause a problem.

@nicolasff
Copy link
Member

Thanks for looking into it. I'll try PHP 5.4 to figure out what the issue is.

@NiroSugir
Copy link
Author

Thank you, Nicolas. Please don't hesitate to ask if you want me to verify anything.

@chobie
Copy link

chobie commented Feb 4, 2012

Hi, @derniro

phpredis use number_format php function as double to string convertor. that function has modified on 5.4
that's the problem.

http://jp2.php.net/manual/en/function.number-format.php

echo number_format(microtime(true),"8",".", "\0");

you can reproduce above code.

let me try latest php5.4. i'll report php bug tracker if that still has issue.

@chobie
Copy link

chobie commented Feb 4, 2012

it reproduced latest 5.4. so I reported that. thx.

https://bugs.php.net/bug.php?id=60977

@NiroSugir
Copy link
Author

Thanks Chobie, looks like I'll be sticking with php 5.3 for a while.

@ihsw
Copy link

ihsw commented May 3, 2012

Is there any progress on this issue? Currently it makes Redis::zAdd() nearly useless in php-5.4.

@leafpeak
Copy link

leafpeak commented May 4, 2012

I would like to know also, any progress on this issue with php 5.4?

@chobie
Copy link

chobie commented May 4, 2012

Sorry guys, I completely forget about this.

Short time solution: you can fix this issue with this patch.
https://gist.github.com/2591440

_php_math_number_format API has broken on PHP5.4. So we have to use another function to convert double to char*.

@nicolasff
Copy link
Member

Thanks @chobie - closing this ticket now that a workaround has been provided for this PHP bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants