Skip to content
This repository
Browse code

Scripting: Force SORT BY constant determinism inside SORT itself.

SORT is able to return (faster than when ordering) unordered output if
the "BY" clause is used with a constant value. However we try to play
well with scripting requirements of determinism providing always sorted
outputs when SORT (and other similar commands) are called by Lua
scripts.

However we used the general mechanism in place in scripting in order to
reorder SORT output, that is, if the command has the "S" flag set, the
Lua scripting engine will take an additional step when converting a
multi bulk reply to Lua value, calling a Lua sorting function.

This is suboptimal as we can do it faster inside SORT itself.
This is also broken as issue #545 shows us: basically when SORT is used
with a constant BY, and additionally also GET is used, the Lua scripting
engine was trying to order the output as a flat array, while it was
actually a list of key-value pairs.

What we do know is to recognized if the caller of SORT is the Lua client
(since we can check this using the REDIS_LUA_CLIENT flag). If so, and if
a "don't sort" condition is triggered by the BY option with a constant
string, we force the lexicographical sorting.

This commit fixes this bug and improves the performance, and at the same
time simplifies the implementation. This does not mean I'm smart today,
it means I was stupid when I committed the original implementation ;)
  • Loading branch information...
commit 5ddee9b7d5885ae5650484bc55d89d7de54ee3a8 1 parent fd2a895
Salvatore Sanfilippo authored
2  src/redis.c
@@ -222,7 +222,7 @@ struct redisCommand redisCommandTable[] = {
222 222 {"replconf",replconfCommand,-1,"ars",0,NULL,0,0,0,0,0},
223 223 {"flushdb",flushdbCommand,1,"w",0,NULL,0,0,0,0,0},
224 224 {"flushall",flushallCommand,1,"w",0,NULL,0,0,0,0,0},
225   - {"sort",sortCommand,-2,"wmS",0,NULL,1,1,1,0,0},
  225 + {"sort",sortCommand,-2,"wm",0,NULL,1,1,1,0,0},
226 226 {"info",infoCommand,-1,"rlt",0,NULL,0,0,0,0,0},
227 227 {"monitor",monitorCommand,1,"ars",0,NULL,0,0,0,0,0},
228 228 {"ttl",ttlCommand,2,"r",0,NULL,1,1,1,0,0},
1  src/redis.h
@@ -570,7 +570,6 @@ struct redisServer {
570 570 list *unblocked_clients; /* list of clients to unblock before next loop */
571 571 /* Sort parameters - qsort_r() is only available under BSD so we
572 572 * have to take this state global, in order to pass it to sortCompare() */
573   - int sort_dontsort;
574 573 int sort_desc;
575 574 int sort_alpha;
576 575 int sort_bypattern;
2  src/scripting.c
@@ -282,8 +282,6 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) {
282 282 * reply as expected. */
283 283 if ((cmd->flags & REDIS_CMD_SORT_FOR_SCRIPT) &&
284 284 (reply[0] == '*' && reply[1] != '-')) {
285   - /* Skip this step if command is SORT but output was already sorted */
286   - if (cmd->proc != sortCommand || server.sort_dontsort)
287 285 luaSortArray(lua);
288 286 }
289 287 sdsfree(reply);
8 src/sort.c
@@ -215,8 +215,11 @@ void sortCommand(redisClient *c) {
215 215
216 216 /* If we have STORE we need to force sorting for deterministic output
217 217 * and replication. We use alpha sorting since this is guaranteed to
218   - * work with any input. */
219   - if (storekey && dontsort) {
  218 + * work with any input.
  219 + *
  220 + * We also want determinism when SORT is called from Lua scripts, so
  221 + * in this case we also force alpha sorting. */
  222 + if ((storekey || c->flags & REDIS_LUA_CLIENT) && dontsort) {
220 223 dontsort = 0;
221 224 alpha = 1;
222 225 sortby = NULL;
@@ -326,7 +329,6 @@ void sortCommand(redisClient *c) {
326 329 }
327 330 if (end >= vectorlen) end = vectorlen-1;
328 331
329   - server.sort_dontsort = dontsort;
330 332 if (dontsort == 0) {
331 333 server.sort_desc = desc;
332 334 server.sort_alpha = alpha;
8 tests/unit/scripting.tcl
@@ -203,23 +203,23 @@ start_server {tags {"scripting"}} {
203 203 r eval {return redis.call('smembers','myset')} 0
204 204 } {a aa aaa azz b c d e f g h i l m n o p q r s t u v z}
205 205
206   - test "SORT is normally not re-ordered by the scripting engine" {
  206 + test "SORT is normally not alpha re-ordered for the scripting engine" {
207 207 r del myset
208 208 r sadd myset 1 2 3 4 10
209 209 r eval {return redis.call('sort','myset','desc')} 0
210 210 } {10 4 3 2 1}
211 211
212   - test "SORT BY <constant> output gets ordered by scripting" {
  212 + test "SORT BY <constant> output gets ordered for scripting" {
213 213 r del myset
214 214 r sadd myset a b c d e f g h i l m n o p q r s t u v z aa aaa azz
215 215 r eval {return redis.call('sort','myset','by','_')} 0
216 216 } {a aa aaa azz b c d e f g h i l m n o p q r s t u v z}
217 217
218   - test "SORT output containing NULLs is well handled by scripting" {
  218 + test "SORT BY <constant> with GET gets ordered for scripting" {
219 219 r del myset
220 220 r sadd myset a b c
221 221 r eval {return redis.call('sort','myset','by','_','get','#','get','_:*')} 0
222   - } {{} {} {} a b c}
  222 + } {a {} b {} c {}}
223 223
224 224 test "redis.sha1hex() implementation" {
225 225 list [r eval {return redis.sha1hex('')} 0] \

0 comments on commit 5ddee9b

Please sign in to comment.
Something went wrong with that request. Please try again.