From 859e5aff85c57647e85b4fb0dcea15f72d9ebfdc Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 16 Sep 2026 19:48:37 +0700 Subject: [PATCH 1/7] fix(sorted_set): reject GT+LT and NaN weights, and report syntax errors as syntax errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two inputs moon ACCEPTED that Redis rejects, plus a sweep of wrong error classes. Every expectation here was taken from a live redis-server 8.6.1 on a second port, command by command, before anything was changed. Accepted where Redis errors: * `ZADD k GT LT 1 m` returned `(integer) 1`. The guard only rejected `nx && (gt || lt)`; Redis rejects all three pairings. Worse, the pairing then reached TWO mutation sites that answered it with a SILENT no-op — the listpack arm and the B+tree arm each carried a `gt && lt => false` fallthrough, so a client asking for an impossible combination got `0` and an unchanged score instead of an error. Both arms are removed rather than left unreachable, so relaxing the guard cannot quietly resurrect the no-op. The message also said "GT, LT, and NX"; Redis says "and/or". * `ZUNIONSTORE d 1 k WEIGHTS nan` returned `(integer) 1`, and the read twin `ZUNION 1 k WEIGHTS nan` returned the member. Rust's `str::parse::` accepts "nan" where C's `strtod` + `isnan` check in `getDoubleFromObjectOrReply` rejects it, and only a `None` mapped to the error. A NaN weight poisons every aggregated score. Infinities stay legal, as they are on Redis. Wrong error CLASS. This matters beyond wording: redis-py raises a distinct exception type per class, so a client branching on the exception takes the wrong branch — and retries a request that can never succeed. command was now ZPOPMIN k notanint / k -1 not an integer value is out of range, must be positive ZINTERCARD 0 k numkeys can't be ... at least 1 input key is needed for 'zintercard' command ZUNIONSTORE d 0 k wrong number of args at least 1 input key is needed for 'zunionstore' command ZUNIONSTORE d -1 k not an integer (same) ZUNIONSTORE d 1 k WEIGHTS wrong number of args syntax error ZUNIONSTORE d 1 k AGGREGATE wrong number of args syntax error ZUNIONSTORE d 2 k wrong number of args syntax error ZADD k 1 a 2 wrong number of args syntax error ZMPOP 0 k MIN numkeys can't be ... numkeys should be greater than 0 The set-operation family splits into TWO classes and the split is the point: `getLongFromObjectOrReply(..., NULL)` answers the generic integer error for bytes that are not a number, and only then does `if (setnum < 1)` name the command. ZMPOP does NOT split — `getRangeLongFromObject` carries one message for every failure. Arity is checked FIRST, so `ZUNION 0`, which names no key at all, stays an arity error and never reaches the numkeys rules. Two divergences found while reproducing the above, in neither issue: * `ZINTERCARD k LIMIT -1` — `limit: usize` failed a negative into the generic message; Redis says `LIMIT can't be negative`. * `ZMPOP 1 k MIN COUNT 0` / `COUNT -1` — Redis says `count should be greater than 0`. And one moon#967 leftover: that sweep rewrote every zset option loop to reject an unrecognised token except `zstore_impl`, so `ZUNIONSTORE d 1 k BOGUS` stepped over `BOGUS` and answered a different, successful command. Deliberately NOT changed. moon#969 also cites four ZRANGE-family sites as wrong. They are not. `mod.rs:317/:329/:639/:649` are rank-index parses, and the `LIMIT offset count` parses next to them take the same shape; Redis reads all of them with `getLongFromObjectOrReply(..., NULL)`, which produces exactly the `ERR value is not an integer or out of range` moon already answers — confirmed against the oracle, which disagrees with the issue. Those sites also accept negatives, which the new helpers do not. `rank_and_limit_parses_keep_the_ generic_integer_error` pins all eight so a later reading of moon#969 cannot "fix" them into a divergence. Two shared helpers carry the two shapes so eleven call sites do not each transcribe them, and the boundary against the correct sites is explicit: `parse_numkeys` (two classes, names the command) and `parse_bounded_count` (one bespoke message, minimum bound). The ZADD option loop itself changes by one condition and one error class only. BEHAVIOUR CHANGE: `ZADD ... GT LT` and `WEIGHTS nan` now error where they previously succeeded. Closes #969 author: Tin Dang --- src/command/sorted_set/mod.rs | 377 +++++++++++++++++++++ src/command/sorted_set/sorted_set_read.rs | 68 ++-- src/command/sorted_set/sorted_set_write.rs | 142 +++++--- 3 files changed, 509 insertions(+), 78 deletions(-) diff --git a/src/command/sorted_set/mod.rs b/src/command/sorted_set/mod.rs index a5566cfda..761e24664 100644 --- a/src/command/sorted_set/mod.rs +++ b/src/command/sorted_set/mod.rs @@ -60,6 +60,75 @@ pub(super) enum AggregateOp { Max, } +// --------------------------------------------------------------------------- +// Argument validation shared by the read and write halves (moon#969) +// --------------------------------------------------------------------------- +// +// The error CLASS matters beyond the wording. redis-py maps each class to a +// distinct exception type, so a client that branches on the exception takes +// the WRONG branch when moon answers an arity error where Redis answers a +// syntax error — and retries a request that can never succeed. + +/// `ERR at least 1 input key is needed for '' command`. +/// +/// Redis interpolates the command's registered (lower-case) name here, exactly +/// as `err_wrong_args` does for the arity message and for the same reason: +/// clients string-match the result. +fn err_at_least_one_key(cmd: &str) -> Frame { + const PREFIX: &str = "ERR at least 1 input key is needed for '"; + const SUFFIX: &str = "' command"; + let mut msg = String::with_capacity(PREFIX.len() + cmd.len() + SUFFIX.len()); + msg.push_str(PREFIX); + msg.extend(cmd.chars().map(|c| c.to_ascii_lowercase())); + msg.push_str(SUFFIX); + Frame::Error(Bytes::from(msg)) +} + +/// The `numkeys` contract of the set-operation family — ZUNIONSTORE, +/// ZINTERSTORE, ZUNION, ZINTER, ZDIFF, ZINTERCARD. +/// +/// Redis's `zunionInterDiffGenericCommand` answers in TWO classes, and the +/// split is the whole point: `getLongFromObjectOrReply(…, NULL)` reports +/// `ERR value is not an integer or out of range` for bytes that are not a +/// number, and only then does `if (setnum < 1)` report `at least 1 input key +/// is needed …`. Parsing straight into a `usize` collapsed the two — `-1` came +/// back as the integer error, and `0` came back as an ARITY error, a third +/// class again. Verified against redis-server 8.6.1. +pub(super) fn parse_numkeys(arg: &[u8], cmd: &str) -> Result { + let n: i64 = match std::str::from_utf8(arg).ok().and_then(|s| s.parse().ok()) { + Some(n) => n, + None => return Err(err("ERR value is not an integer or out of range")), + }; + if n < 1 { + return Err(err_at_least_one_key(cmd)); + } + Ok(n as usize) +} + +/// A count argument Redis reads with `getRangeLongFromObject` / +/// `getPositiveLongFromObject`: ONE bespoke message for EVERY failure, whether +/// the bytes were not a number at all or the number was below `min`. +/// +/// Deliberately the opposite shape to [`parse_numkeys`]. ZPOPMIN/ZPOPMAX's +/// `count`, ZMPOP's `numkeys` and `COUNT`, and ZINTERCARD's `LIMIT` each carry +/// a message Redis hands to that one call site; moon answered the generic +/// integer error at all of them. +/// +/// Note what this is NOT for. A ZRANGE **rank index**, or a +/// `LIMIT offset count` pair, is read with `NULL` as the message, so the +/// generic `ERR value is not an integer or out of range` is the CORRECT reply +/// there — and those sites accept negatives. They are already right and do not +/// come through here (moon#969 cites them; the oracle says otherwise). +pub(super) fn parse_bounded_count(arg: &[u8], min: i64, msg: &str) -> Result { + match std::str::from_utf8(arg) + .ok() + .and_then(|s| s.parse::().ok()) + { + Some(n) if n >= min => Ok(n as usize), + _ => Err(err(msg)), + } +} + // --------------------------------------------------------------------------- // Internal helpers -- CRITICAL for dual structure consistency // --------------------------------------------------------------------------- @@ -3250,6 +3319,314 @@ mod tests { ); } + // ---- moon#969 / moon#792: option semantics and error classes ---------- + // + // Every expectation below was taken from a live redis-server 8.6.1 on a + // second port, command by command, BEFORE any of it was changed. + + /// A member short enough to stay in the listpack encoding. + const LP_MEMBER: &[u8] = b"m"; + /// A member past `zset-max-listpack-value` (64), which forces the B+tree. + /// The two `ZADD` mutation loops are SEPARATE code, so every claim about + /// flags or `CH` has to be made twice. + const BT_MEMBER: &[u8] = + b"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + + fn error_text(f: &Frame) -> String { + match f { + Frame::Error(e) => String::from_utf8_lossy(e).into_owned(), + other => panic!("expected an error, got {other:?}"), + } + } + + #[test] + fn gt_lt_and_nx_are_pairwise_incompatible() { + let mut db = Database::new(); + const MSG: &str = "ERR GT, LT, and/or NX options at the same time are not compatible"; + + // `GT LT` is the pairing the old guard missed: it was ACCEPTED, and + // then the mutation loops answered it with a silent no-op. + for flags in [ + [&b"GT"[..], &b"LT"[..]], + [&b"GT"[..], &b"NX"[..]], + [&b"LT"[..], &b"NX"[..]], + ] { + let reply = run_zadd(&mut db, &[b"k", flags[0], flags[1], b"1", b"m"]); + assert_eq!(error_text(&reply), MSG, "flags {flags:?}"); + } + assert_eq!( + run_zadd(&mut db, &[b"k", b"GT", b"LT", b"NX", b"1", b"m"]), + Frame::Error(Bytes::from_static(MSG.as_bytes())) + ); + + // Rejected before the keyspace is touched, on BOTH encodings. + assert_eq!(run_zcard(&mut db, &[b"k"]), Frame::Integer(0)); + for member in [LP_MEMBER, BT_MEMBER] { + run_zadd(&mut db, &[b"z", b"5", member]); + let reply = run_zadd(&mut db, &[b"z", b"GT", b"LT", b"9", member]); + assert_eq!(error_text(&reply), MSG); + assert_eq!( + run_zscore(&mut db, &[b"z", member]), + Frame::BulkString(Bytes::from_static(b"5")), + "a rejected GT+LT must not have rescored the member" + ); + } + + // The pairing that is still legal on its own keeps working. + assert_eq!( + run_zadd(&mut db, &[b"z", b"GT", b"9", LP_MEMBER]), + Frame::Integer(0) + ); + assert_eq!( + run_zscore(&mut db, &[b"z", LP_MEMBER]), + Frame::BulkString(Bytes::from_static(b"9")) + ); + } + + #[test] + fn an_odd_score_member_tail_is_a_syntax_error() { + let mut db = Database::new(); + // Redis splits these two: NO pairs at all fails `commandCheckArity`, + // an ODD tail fails inside `zaddGenericCommand`. + assert_eq!( + error_text(&run_zadd(&mut db, &[b"k", b"1", b"a", b"2"])), + "ERR syntax error" + ); + assert_eq!( + error_text(&run_zadd(&mut db, &[b"k", b"CH", b"1"])), + "ERR syntax error" + ); + assert_eq!( + error_text(&run_zadd(&mut db, &[b"k", b"NX"])), + "ERR wrong number of arguments for 'zadd' command" + ); + assert_eq!(run_zcard(&mut db, &[b"k"]), Frame::Integer(0)); + } + + #[test] + fn a_nan_weight_is_not_a_float() { + let mut db = Database::new(); + run_zadd(&mut db, &[b"src", b"1", b"a"]); + let nan_args = + |extra: &[&[u8]]| -> Vec { extra.iter().map(|a| bulk(a)).collect::>() }; + + // Rust parses "nan"; C's `strtod` + `isnan` check does not. + for w in [&b"nan"[..], &b"-nan"[..], &b"NaN"[..]] { + assert_eq!( + error_text(&zunionstore( + &mut db, + &nan_args(&[b"d", b"1", b"src", b"WEIGHTS", w]) + )), + "ERR weight value is not a float", + "ZUNIONSTORE weight {}", + String::from_utf8_lossy(w) + ); + assert_eq!( + error_text(&zunion(&mut db, &nan_args(&[b"1", b"src", b"WEIGHTS", w]))), + "ERR weight value is not a float", + "ZUNION weight {}", + String::from_utf8_lossy(w) + ); + } + // …and the destination was never written. + assert_eq!(run_zcard(&mut db, &[b"d"]), Frame::Integer(0)); + + // An INFINITE weight stays legal, exactly as on Redis. + assert_eq!( + zunionstore( + &mut db, + &nan_args(&[b"d", b"1", b"src", b"WEIGHTS", b"inf"]) + ), + Frame::Integer(1) + ); + } + + #[test] + fn count_and_numkeys_errors_carry_the_class_redis_uses() { + let mut db = Database::new(); + run_zadd(&mut db, &[b"z", b"1", b"a", b"2", b"b"]); + let f = |args: &[&[u8]]| -> Vec { args.iter().map(|a| bulk(a)).collect() }; + + // `getPositiveLongFromObject` — one message for every failure. + for bad in [&b"notanint"[..], &b"-1"[..], &b"1.5"[..]] { + assert_eq!( + error_text(&zpopmin(&mut db, &f(&[b"z", bad]))), + "ERR value is out of range, must be positive" + ); + assert_eq!( + error_text(&zpopmax(&mut db, &f(&[b"z", bad]))), + "ERR value is out of range, must be positive" + ); + } + assert_eq!(run_zcard(&mut db, &[b"z"]), Frame::Integer(2)); + + // The set-operation family SPLITS: not-a-number is the generic integer + // error, a number below 1 names the command. + assert_eq!( + error_text(&zunionstore(&mut db, &f(&[b"d", b"notanint", b"z"]))), + "ERR value is not an integer or out of range" + ); + for bad in [&b"0"[..], &b"-1"[..]] { + assert_eq!( + error_text(&zunionstore(&mut db, &f(&[b"d", bad, b"z"]))), + "ERR at least 1 input key is needed for 'zunionstore' command" + ); + assert_eq!( + error_text(&zinterstore(&mut db, &f(&[b"d", bad, b"z"]))), + "ERR at least 1 input key is needed for 'zinterstore' command" + ); + assert_eq!( + error_text(&zunion(&mut db, &f(&[bad, b"z"]))), + "ERR at least 1 input key is needed for 'zunion' command" + ); + assert_eq!( + error_text(&zinter(&mut db, &f(&[bad, b"z"]))), + "ERR at least 1 input key is needed for 'zinter' command" + ); + assert_eq!( + error_text(&zdiff(&mut db, &f(&[bad, b"z"]))), + "ERR at least 1 input key is needed for 'zdiff' command" + ); + assert_eq!( + error_text(&zintercard(&mut db, &f(&[bad, b"z"]))), + "ERR at least 1 input key is needed for 'zintercard' command" + ); + } + // Arity is checked FIRST, so a form that names no key at all never + // reaches the numkeys rules. + assert_eq!( + error_text(&zunion(&mut db, &f(&[b"0"]))), + "ERR wrong number of arguments for 'zunion' command" + ); + assert_eq!( + error_text(&zintercard(&mut db, &f(&[b"0"]))), + "ERR wrong number of arguments for 'zintercard' command" + ); + + // ZMPOP does NOT split — `getRangeLongFromObject` with one message. + for bad in [&b"0"[..], &b"-1"[..], &b"notanint"[..]] { + assert_eq!( + error_text(&zmpop(&mut db, &f(&[bad, b"z", b"MIN"]))), + "ERR numkeys should be greater than 0" + ); + } + for bad in [&b"0"[..], &b"-1"[..], &b"notanint"[..]] { + assert_eq!( + error_text(&zmpop(&mut db, &f(&[b"1", b"z", b"MIN", b"COUNT", bad]))), + "ERR count should be greater than 0" + ); + } + assert_eq!( + run_zcard(&mut db, &[b"z"]), + Frame::Integer(2), + "a rejected ZMPOP must not have popped" + ); + + // ZINTERCARD's LIMIT has its own message too. + for bad in [&b"-1"[..], &b"notanint"[..]] { + assert_eq!( + error_text(&zintercard(&mut db, &f(&[b"1", b"z", b"LIMIT", bad]))), + "ERR LIMIT can't be negative" + ); + } + assert_eq!( + zintercard(&mut db, &f(&[b"1", b"z", b"LIMIT", b"0"])), + Frame::Integer(2) + ); + + // A numkeys that overruns the key list, a short WEIGHTS list, a + // dangling AGGREGATE and an unknown trailing token are all + // `syntax error` — NOT arity errors. + let syntax = "ERR syntax error"; + assert_eq!( + error_text(&zunionstore(&mut db, &f(&[b"d", b"2", b"z"]))), + syntax + ); + assert_eq!(error_text(&zunion(&mut db, &f(&[b"2", b"z"]))), syntax); + assert_eq!(error_text(&zintercard(&mut db, &f(&[b"2", b"z"]))), syntax); + assert_eq!( + error_text(&zmpop(&mut db, &f(&[b"2", b"z", b"MIN"]))), + syntax + ); + assert_eq!( + error_text(&zunionstore(&mut db, &f(&[b"d", b"1", b"z", b"WEIGHTS"]))), + syntax + ); + assert_eq!( + error_text(&zunion(&mut db, &f(&[b"1", b"z", b"WEIGHTS"]))), + syntax + ); + assert_eq!( + error_text(&zunionstore(&mut db, &f(&[b"d", b"1", b"z", b"AGGREGATE"]))), + syntax + ); + // moon#967 rewrote every zset option loop but this one. + assert_eq!( + error_text(&zunionstore(&mut db, &f(&[b"d", b"1", b"z", b"BOGUS"]))), + syntax + ); + assert_eq!( + error_text(&zintercard(&mut db, &f(&[b"1", b"z", b"LIMIT"]))), + syntax + ); + assert_eq!( + error_text(&zmpop(&mut db, &f(&[b"1", b"z", b"MIN", b"COUNT"]))), + syntax + ); + } + + /// moon#969 cites four ZRANGE-family sites as wrong. They are NOT: Redis + /// reads a rank index and a `LIMIT offset count` with + /// `getLongFromObjectOrReply(…, NULL)`, whose message is exactly the + /// generic one moon already answers. This test pins them so the moon#969 + /// sweep cannot "fix" them into a divergence. + #[test] + fn rank_and_limit_parses_keep_the_generic_integer_error() { + let mut db = Database::new(); + run_zadd(&mut db, &[b"z", b"1", b"a", b"2", b"b"]); + let f = |args: &[&[u8]]| -> Vec { args.iter().map(|a| bulk(a)).collect() }; + const GENERIC: &str = "ERR value is not an integer or out of range"; + + assert_eq!( + error_text(&zrange(&mut db, &f(&[b"z", b"notanint", b"5"]))), + GENERIC + ); + assert_eq!( + error_text(&zrange(&mut db, &f(&[b"z", b"0", b"notanint"]))), + GENERIC + ); + assert_eq!( + error_text(&zrange(&mut db, &f(&[b"z", b"1.5", b"2"]))), + GENERIC + ); + assert_eq!( + error_text(&zrevrange(&mut db, &f(&[b"z", b"notanint", b"5"]))), + GENERIC + ); + assert_eq!( + error_text(&zrangebyscore( + &mut db, + &f(&[b"z", b"0", b"5", b"LIMIT", b"notanint", b"5"]) + )), + GENERIC + ); + assert_eq!( + error_text(&zrevrangebyscore( + &mut db, + &f(&[b"z", b"5", b"0", b"LIMIT", b"notanint", b"5"]) + )), + GENERIC + ); + assert_eq!( + error_text(&zrandmember(&mut db, &f(&[b"z", b"notanint"]))), + GENERIC + ); + assert_eq!( + error_text(&zrangestore(&mut db, &f(&[b"d", b"z", b"notanint", b"5"]))), + GENERIC + ); + } + /// moon#967. Redis defines a negative LIMIT offset as "return nothing". /// moon parsed it as a plain i64 and clamped it to 0 with `.max(0)`, /// returning a non-empty result. diff --git a/src/command/sorted_set/sorted_set_read.rs b/src/command/sorted_set/sorted_set_read.rs index 4d17f8f5b..eae714e8a 100644 --- a/src/command/sorted_set/sorted_set_read.rs +++ b/src/command/sorted_set/sorted_set_read.rs @@ -12,8 +12,8 @@ use std::collections::HashMap; use super::{ AggregateOp, clamp_nan_to_zero, format_score, format_score_bytes, glob_match, lex_in_range, - parse_lex_bound, parse_score_bound, zrange_by_lex, zrange_by_rank, zrange_by_score, - zrange_from_entries, + parse_bounded_count, parse_lex_bound, parse_numkeys, parse_score_bound, zrange_by_lex, + zrange_by_rank, zrange_by_score, zrange_from_entries, }; // --------------------------------------------------------------------------- @@ -921,23 +921,22 @@ fn parse_setop_args( cmd_name: &str, supports_weights: bool, ) -> Result<(Vec, Vec, AggregateOp, bool), Frame> { - if args.is_empty() { + // Redis checks ARITY first, so `ZUNION 0` — which never names a key — is + // an arity error and never reaches the `numkeys` rules below (moon#969). + // These commands are declared `-3`: numkeys plus at least one key. + if args.len() < 2 { return Err(err_wrong_args(cmd_name)); } let numkeys_bytes = match extract_bytes(&args[0]) { Some(b) => b, None => return Err(err_wrong_args(cmd_name)), }; - let numkeys: usize = match std::str::from_utf8(numkeys_bytes) - .ok() - .and_then(|s| s.parse().ok()) - { - Some(n) if n > 0 => n, - _ => return Err(err("ERR value is not an integer or out of range")), - }; + let numkeys = parse_numkeys(numkeys_bytes, cmd_name)?; + // Past the arity floor, a `numkeys` that overruns the key list is + // `syntax error` (moon#969). if args.len() < 1 + numkeys { - return Err(err_wrong_args(cmd_name)); + return Err(err("ERR syntax error")); } let keys: Vec = (0..numkeys) @@ -963,14 +962,22 @@ fn parse_setop_args( }; if supports_weights && opt.eq_ignore_ascii_case(b"WEIGHTS") { for w in 0..numkeys { + // Too few weights to cover the key list is `syntax error` + // (moon#969), not an arity error. if i + 1 + w >= args.len() { - return Err(err_wrong_args(cmd_name)); + return Err(err("ERR syntax error")); } let wb = match extract_bytes(&args[i + 1 + w]) { Some(b) => b, - None => return Err(err_wrong_args(cmd_name)), + None => return Err(err("ERR syntax error")), }; - let wval: f64 = match std::str::from_utf8(wb).ok().and_then(|s| s.parse().ok()) { + // `"nan"` parses in Rust; Redis's `getDoubleFromObjectOrReply` + // rejects it after `strtod` (moon#969). Infinities stay legal. + let wval: f64 = match std::str::from_utf8(wb) + .ok() + .and_then(|s| s.parse::().ok()) + .filter(|v| !v.is_nan()) + { Some(v) => v, None => return Err(err("ERR weight value is not a float")), }; @@ -979,11 +986,11 @@ fn parse_setop_args( i += 1 + numkeys; } else if supports_weights && opt.eq_ignore_ascii_case(b"AGGREGATE") { if i + 1 >= args.len() { - return Err(err_wrong_args(cmd_name)); + return Err(err("ERR syntax error")); } let agg_b = match extract_bytes(&args[i + 1]) { Some(b) => b.as_ref(), - None => return Err(err_wrong_args(cmd_name)), + None => return Err(err("ERR syntax error")), }; aggregate = if agg_b.eq_ignore_ascii_case(b"SUM") { AggregateOp::Sum @@ -1256,22 +1263,21 @@ pub fn zinter_readonly(db: &Database, args: &[Frame], now_ms: u64) -> Frame { /// ZINTERCARD numkeys key [key …] [LIMIT limit] — read-only twin. pub fn zintercard_readonly(db: &Database, args: &[Frame], now_ms: u64) -> Frame { - if args.is_empty() { + // Arity before everything else — `ZINTERCARD 0` names no key and is an + // arity error on Redis, not a numkeys error (moon#969). + if args.len() < 2 { return err_wrong_args("ZINTERCARD"); } let numkeys_bytes = match extract_bytes(&args[0]) { Some(b) => b, None => return err_wrong_args("ZINTERCARD"), }; - let numkeys: usize = match std::str::from_utf8(numkeys_bytes) - .ok() - .and_then(|s| s.parse().ok()) - { - Some(n) if n > 0 => n, - _ => return err("ERR numkeys can't be non-positive value"), + let numkeys = match parse_numkeys(numkeys_bytes, "ZINTERCARD") { + Ok(n) => n, + Err(e) => return e, }; if args.len() < 1 + numkeys { - return err_wrong_args("ZINTERCARD"); + return err("ERR syntax error"); } let keys: Vec = (0..numkeys) .map(|j| { @@ -1292,15 +1298,19 @@ pub fn zintercard_readonly(db: &Database, args: &[Frame], now_ms: u64) -> Frame }; if opt.eq_ignore_ascii_case(b"LIMIT") { if i + 1 >= args.len() { - return err_wrong_args("ZINTERCARD"); + return err("ERR syntax error"); } let lb = match extract_bytes(&args[i + 1]) { Some(b) => b, - None => return err_wrong_args("ZINTERCARD"), + None => return err("ERR syntax error"), }; - limit = match std::str::from_utf8(lb).ok().and_then(|s| s.parse().ok()) { - Some(v) => v, - None => return err("ERR value is not an integer or out of range"), + // `getPositiveLongFromObject(…, "LIMIT can't be negative")`: one + // message for a negative AND for bytes that are not a number + // (moon#969). `limit: usize` used to fail a negative into the + // generic integer error. + limit = match parse_bounded_count(lb, 0, "ERR LIMIT can't be negative") { + Ok(v) => v, + Err(e) => return e, }; i += 2; } else { diff --git a/src/command/sorted_set/sorted_set_write.rs b/src/command/sorted_set/sorted_set_write.rs index 3b39c768a..37e99c049 100644 --- a/src/command/sorted_set/sorted_set_write.rs +++ b/src/command/sorted_set/sorted_set_write.rs @@ -11,8 +11,9 @@ use crate::command::helpers::{all_args_are_bytes, err, err_wrong_args, extract_b use crate::command::sorted_set::work_budget; use super::{ - AggregateOp, clamp_nan_to_zero, format_score, format_score_bytes, zadd_member, zrange_by_lex, - zrange_by_rank, zrange_by_score, zrem_member, zset_insert_absent, zset_update_existing, + AggregateOp, clamp_nan_to_zero, format_score, format_score_bytes, parse_bounded_count, + parse_numkeys, zadd_member, zrange_by_lex, zrange_by_rank, zrange_by_score, zrem_member, + zset_insert_absent, zset_update_existing, }; // --------------------------------------------------------------------------- @@ -149,16 +150,25 @@ pub fn zadd(db: &mut Database, args: &[Frame]) -> Frame { if nx && xx { return err("ERR XX and NX options at the same time are not compatible"); } - // NX and GT/LT are not compatible - if nx && (gt || lt) { - return err("ERR GT, LT, and NX options at the same time are not compatible"); + // GT, LT and NX are pairwise incompatible (moon#969). The old guard only + // caught `nx && (gt || lt)`, so `GT LT` was ACCEPTED and then silently did + // nothing — Redis's `zaddGenericCommand` rejects all three pairings, and + // its message says "and/or". + if (gt && nx) || (lt && nx) || (gt && lt) { + return err("ERR GT, LT, and/or NX options at the same time are not compatible"); } // Remaining args must be score member pairs let remaining = &args[i..]; - if remaining.is_empty() || !remaining.len().is_multiple_of(2) { + if remaining.is_empty() { return err_wrong_args("ZADD"); } + // An ODD tail is a different class from NO tail (moon#969): Redis fails + // `ZADD k 1 a 2` in `zaddGenericCommand` with `syntax error`, and only a + // command with no pairs at all (`ZADD k NX`) trips `commandCheckArity`. + if !remaining.len().is_multiple_of(2) { + return err("ERR syntax error"); + } // moon#814: validate EVERY pair BEFORE touching the keyspace. // @@ -313,10 +323,14 @@ pub fn zadd(db: &mut Database, args: &[Frame]) -> Frame { work_budget::note_stored_score_parse(); let old = current.as_score().unwrap_or(0.0); old_score = old; + // `gt && lt` no longer reaches here: the guard above + // rejects that pairing outright (moon#969). The arm + // that used to sit between `nx` and `gt` answered + // `false` — a SILENT no-op for a command Redis + // refuses — and is gone rather than left unreachable, + // so relaxing the guard cannot quietly resurrect it. let should_update = if nx { false // NX: never update existing - } else if gt && lt { - false // GT+LT together: never update } else if gt { score > old } else if lt { @@ -432,10 +446,12 @@ pub fn zadd(db: &mut Database, args: &[Frame]) -> Frame { // so the score never has to be looked up a second time to write it. let mut accepted = false; let existing_score: Option = zset_update_existing(members, scores, member, |old| { + // The second of the two `gt && lt` fallthroughs moon#969 left + // standing (the listpack arm has the other). The guard at the top + // of `zadd` now rejects the pairing, so this arm was both dead and + // wrong; it is removed rather than left unreachable. let should_update = if nx { false // NX: never update existing - } else if gt && lt { - false // GT+LT together: never update (mutually exclusive) } else if gt { score > old } else if lt { @@ -801,12 +817,17 @@ pub fn zpopmin(db: &mut Database, args: &[Frame]) -> Frame { Some(b) => b, None => return err_wrong_args("ZPOPMIN"), }; - match std::str::from_utf8(count_bytes) - .ok() - .and_then(|s| s.parse::().ok()) - { - Some(c) if c >= 0 => c as usize, - _ => return err("ERR value is not an integer or out of range"), + // moon#969: Redis reads this with `getPositiveLongFromObject`, which + // carries its OWN message for every failure — non-numeric and + // negative alike. moon answered the generic integer error, a + // different exception type in every client that maps them. + match parse_bounded_count( + count_bytes, + 0, + "ERR value is out of range, must be positive", + ) { + Ok(c) => c, + Err(e) => return e, } } else { 1 @@ -867,12 +888,17 @@ pub fn zpopmax(db: &mut Database, args: &[Frame]) -> Frame { Some(b) => b, None => return err_wrong_args("ZPOPMAX"), }; - match std::str::from_utf8(count_bytes) - .ok() - .and_then(|s| s.parse::().ok()) - { - Some(c) if c >= 0 => c as usize, - _ => return err("ERR value is not an integer or out of range"), + // moon#969: Redis reads this with `getPositiveLongFromObject`, which + // carries its OWN message for every failure — non-numeric and + // negative alike. moon answered the generic integer error, a + // different exception type in every client that maps them. + match parse_bounded_count( + count_bytes, + 0, + "ERR value is out of range, must be positive", + ) { + Ok(c) => c, + Err(e) => return e, } } else { 1 @@ -945,16 +971,16 @@ fn zstore_impl(db: &mut Database, args: &[Frame], intersect: bool) -> Frame { Some(b) => b, None => return err_wrong_args(cmd_name), }; - let numkeys: usize = match std::str::from_utf8(numkeys_bytes) - .ok() - .and_then(|s| s.parse().ok()) - { - Some(n) => n, - None => return err("ERR value is not an integer or out of range"), + let numkeys = match parse_numkeys(numkeys_bytes, cmd_name) { + Ok(n) => n, + Err(e) => return e, }; - if numkeys == 0 || args.len() < 2 + numkeys { - return err_wrong_args(cmd_name); + // A `numkeys` that overruns the key list is `syntax error`, not an arity + // error (moon#969) — Redis's arity check already passed above, and + // `zunionInterDiffGenericCommand` answers `shared.syntaxerr` here. + if args.len() < 2 + numkeys { + return err("ERR syntax error"); } // Collect source keys @@ -981,14 +1007,24 @@ fn zstore_impl(db: &mut Database, args: &[Frame], intersect: bool) -> Frame { }; if opt.eq_ignore_ascii_case(b"WEIGHTS") { for w in 0..numkeys { + // Too few weights to cover the key list is `syntax error` on + // Redis, not an arity error (moon#969). if i + 1 + w >= args.len() { - return err_wrong_args(cmd_name); + return err("ERR syntax error"); } let wb = match extract_bytes(&args[i + 1 + w]) { Some(b) => b, - None => return err_wrong_args(cmd_name), + None => return err("ERR syntax error"), }; - let wval: f64 = match std::str::from_utf8(wb).ok().and_then(|s| s.parse().ok()) { + // `"nan"` PARSES in Rust where C's `strtod` + `isnan` check in + // `getDoubleFromObjectOrReply` rejects it (moon#969), so a NaN + // weight sailed through and poisoned every aggregated score. + // Infinities stay legal, as they are on Redis. + let wval: f64 = match std::str::from_utf8(wb) + .ok() + .and_then(|s| s.parse::().ok()) + .filter(|v| !v.is_nan()) + { Some(v) => v, None => return err("ERR weight value is not a float"), }; @@ -997,11 +1033,11 @@ fn zstore_impl(db: &mut Database, args: &[Frame], intersect: bool) -> Frame { i += 1 + numkeys; } else if opt.eq_ignore_ascii_case(b"AGGREGATE") { if i + 1 >= args.len() { - return err_wrong_args(cmd_name); + return err("ERR syntax error"); } let agg_b = match extract_bytes(&args[i + 1]) { Some(b) => b.as_ref(), - None => return err_wrong_args(cmd_name), + None => return err("ERR syntax error"), }; aggregate = if agg_b.eq_ignore_ascii_case(b"SUM") { AggregateOp::Sum @@ -1014,7 +1050,11 @@ fn zstore_impl(db: &mut Database, args: &[Frame], intersect: bool) -> Frame { }; i += 2; } else { - i += 1; + // moon#967 rewrote every OTHER zset option loop to reject an + // unrecognised token and missed this one, so `ZUNIONSTORE d 1 k + // BOGUS` stepped over `BOGUS` and answered a DIFFERENT, successful + // command. Redis: `ERR syntax error`. + return err("ERR syntax error"); } } @@ -1295,16 +1335,18 @@ pub fn zmpop(db: &mut Database, args: &[Frame]) -> Frame { Some(b) => b, None => return err_wrong_args("ZMPOP"), }; - let numkeys: usize = match std::str::from_utf8(numkeys_bytes) - .ok() - .and_then(|s| s.parse().ok()) - { - Some(n) if n > 0 => n, - _ => return err("ERR numkeys can't be non-positive value"), - }; + // ZMPOP does NOT take the two-class split the ZUNIONSTORE family takes: + // Redis reads it with `getRangeLongFromObject(…, 1, LONG_MAX, …, + // "numkeys should be greater than 0")`, one message for every failure + // (moon#969). Verified against redis-server 8.6.1, including `notanint`. + let numkeys = + match parse_bounded_count(numkeys_bytes, 1, "ERR numkeys should be greater than 0") { + Ok(n) => n, + Err(e) => return e, + }; if args.len() < 1 + numkeys + 1 { - return err_wrong_args("ZMPOP"); + return err("ERR syntax error"); } let keys: Vec = (0..numkeys) @@ -1340,16 +1382,18 @@ pub fn zmpop(db: &mut Database, args: &[Frame]) -> Frame { } }; if opt.eq_ignore_ascii_case(b"COUNT") { + // A dangling `COUNT` is `syntax error` (moon#969), the same class + // the bare-token arm below already answers. if i + 1 >= args.len() { - return err_wrong_args("ZMPOP"); + return err("ERR syntax error"); } let cb = match extract_bytes(&args[i + 1]) { Some(b) => b, - None => return err_wrong_args("ZMPOP"), + None => return err("ERR syntax error"), }; - pop_count = match std::str::from_utf8(cb).ok().and_then(|s| s.parse().ok()) { - Some(c) if c > 0 => c, - _ => return err("ERR value is not an integer or out of range"), + pop_count = match parse_bounded_count(cb, 1, "ERR count should be greater than 0") { + Ok(c) => c, + Err(e) => return e, }; i += 2; } else { From b533a255bad33923fe8240ebe98c11d5ad5bcc3b Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 16 Sep 2026 19:48:41 +0700 Subject: [PATCH 2/7] fix(sorted_set): ZADD CH counts a rescore exactly, not against an epsilon window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both ZADD mutation loops decided `changed` with an ABSOLUTE `f64::EPSILON` window — `(old - new).abs() > f64::EPSILON` — where Redis's `zsetAdd` compares the two scores EXACTLY (`if (score != curscore)`). `f64::EPSILON` is the gap between 1.0 and the next double, a RELATIVE quantity. Used as an absolute tolerance it swallows real differences at every magnitude below 1, and the error grows as the scores shrink: ZADD k 0.0000000001 m ZADD k CH 0.00000000010000001 m -> moon: 0 redis: 1 ZSCORE k m -> 0.00000000010000001 The reply said nothing changed; the very next read said otherwise. That is not a rounding nicety — a change of six significant figures was reported as no change, because the ABSOLUTE difference (1e-17) sits under a window sized for magnitude 1. Any client using `CH` as a did-anything-happen signal — cache invalidation, a change feed, a dirty flag — silently skipped the update. The window also disagreed with the code that does the writing. `zset_update_existing` decides whether to move the member on `to_bits()` inequality, so a sub-epsilon rescore really was written to both the hash and the B+tree, and only the tally pretended otherwise. Both arms are fixed, because they are separate code: the listpack loop and the B+tree loop each carried their own copy. `ch_counts_a_sub_epsilon_rescore` drives both — a short member for the listpack encoding, a 73-byte member to force the B+tree — and asserts the reply AND the stored score, so the two can no longer disagree. It uses `nextafter(1.0)`, whose distance from 1.0 is exactly `f64::EPSILON` and so failed the old strict `>`. Neither side can be NaN: `parse_zadd_pair` rejects a NaN score before either loop runs, so `!=` is total here. `-0.0 != 0.0` is false, matching Redis. The range-bound half of the same epsilon defect was fixed separately by moon#966; this is the remaining `CH` counter. Closes #792 author: Tin Dang --- src/command/sorted_set/mod.rs | 37 ++++++++++++++++++++++ src/command/sorted_set/sorted_set_write.rs | 25 ++++++++++++--- 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/src/command/sorted_set/mod.rs b/src/command/sorted_set/mod.rs index 761e24664..a241ed533 100644 --- a/src/command/sorted_set/mod.rs +++ b/src/command/sorted_set/mod.rs @@ -3627,6 +3627,43 @@ mod tests { ); } + /// moon#792. `CH` counted a rescore only when the score moved by MORE than + /// an absolute `f64::EPSILON`, so a real change smaller than ~2.2e-16 was + /// reported as no change — while the stored score really did move, which + /// the `ZSCORE` assertions below prove. Redis's `zsetAdd` compares + /// EXACTLY (`score != curscore`). + #[test] + fn ch_counts_a_sub_epsilon_rescore() { + // `nextafter(1.0)` — the smallest representable move from 1.0, whose + // distance is EXACTLY `f64::EPSILON` and so failed the old `>` test. + const NUDGED: &[u8] = b"1.0000000000000002"; + + for member in [LP_MEMBER, BT_MEMBER] { + let mut db = Database::new(); + assert_eq!(run_zadd(&mut db, &[b"z", b"1", member]), Frame::Integer(1)); + assert_eq!( + run_zadd(&mut db, &[b"z", b"CH", NUDGED, member]), + Frame::Integer(1), + "CH must count a sub-epsilon rescore ({})", + if member == LP_MEMBER { + "listpack" + } else { + "bptree" + } + ); + assert_eq!( + run_zscore(&mut db, &[b"z", member]), + Frame::BulkString(Bytes::from_static(NUDGED)), + "and the score really did move" + ); + // Re-writing the SAME score is still no change. + assert_eq!( + run_zadd(&mut db, &[b"z", b"CH", NUDGED, member]), + Frame::Integer(0) + ); + } + } + /// moon#967. Redis defines a negative LIMIT offset as "return nothing". /// moon parsed it as a plain i64 and clamped it to 0 with `.max(0)`, /// returning a non-empty result. diff --git a/src/command/sorted_set/sorted_set_write.rs b/src/command/sorted_set/sorted_set_write.rs index 37e99c049..c0cd5add3 100644 --- a/src/command/sorted_set/sorted_set_write.rs +++ b/src/command/sorted_set/sorted_set_write.rs @@ -351,7 +351,18 @@ pub fn zadd(db: &mut Database, args: &[Frame]) -> Frame { // `CH` is on the `consults_old` side, so // `old_score` is the real stored score whenever // this tally can be read. - if (old_score - score).abs() > f64::EPSILON { + // + // moon#792: EXACTLY, as Redis's `zsetAdd` does + // (`if (score != curscore)`). An absolute + // `f64::EPSILON` window called any move smaller + // than ~2.2e-16 "unchanged" REGARDLESS of + // magnitude, so rescoring 1e-10 to 1.0000001e-10 — + // a change of six significant figures — replied 0 + // while the stored score really did move, and the + // next read disagreed with the reply. Neither side + // can be NaN: `parse_zadd_pair` rejects a NaN + // score, so `!=` is total here. + if old_score != score { changed += 1; } } @@ -468,9 +479,15 @@ pub fn zadd(db: &mut Database, args: &[Frame]) -> Frame { // `accepted` is the closure's own decision, read back rather // than re-derived: the flag logic has ONE spelling, so the // write and the `CH` tally can never disagree about it. - // `changed` then reports whether the score MOVED, on the same - // epsilon rule this command has always used. - if accepted && (old - score).abs() > f64::EPSILON { + // + // moon#792, the B+tree half: `changed` reports whether the + // score MOVED, compared EXACTLY as Redis's `zsetAdd` does. The + // old absolute `f64::EPSILON` window disagreed with + // `zset_update_existing`, which decides on `to_bits()` — so a + // sub-epsilon rescore really was written to both structures + // and then reported as no change. Neither side can be NaN + // (`parse_zadd_pair` rejects a NaN score). + if accepted && old != score { changed += 1; } } From c3c5e33abee57302435d2366675e98ed1a600395 Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 16 Sep 2026 19:48:41 +0700 Subject: [PATCH 3/7] test(sorted_set): harness rows for the zset error classes and the CH counter Neither harness had a single row for any of these forms, which is exactly how they were free to drift: `scripts/test-consistency.sh` and `scripts/test-commands.sh` both compare moon against a live redis, so a row here is a standing oracle assertion. 69 `assert_both` rows in the consistency suite and 26 `assert_match` rows in the command suite, covering: the three GT/LT/NX pairings and the score being left alone at BOTH encodings; the odd-tail-vs-no-pairs split; a NaN weight on all four set-operation commands plus the infinite weight that stays legal; the ZPOPMIN/ZPOPMAX count class; the two-class numkeys split and the arity floor above it; ZMPOP's single-message numkeys and COUNT; ZINTERCARD's LIMIT; and the six syntax-vs-arity forms. Proven red before the fix, against the pre-fix binary and the same redis 8.6.1 oracle: 48 of the 69 rows FAILED, and the 21 that passed are precisely the anti-regression rows plus the forms that were already correct. All 69 pass after. The rows were extracted verbatim from the committed script to run that comparison, so the evidence cannot have drifted from what landed here. The anti-regression rows are the point of the last block. moon#969 names four ZRANGE-family sites as wrong; the oracle says they are not, because Redis reads a rank index and a `LIMIT offset count` with `getLongFromObjectOrReply(..., NULL)` and that IS the generic integer error moon already answers. Those ten rows passed identically before and after this branch, on both servers, and exist so a later reading of the issue cannot "fix" them into a divergence. CHANGELOG records the behaviour change: `ZADD GT LT` and `WEIGHTS nan` now error where they previously succeeded. Refs #969 Refs #792 author: Tin Dang --- CHANGELOG.md | 41 +++++++++++++ scripts/test-commands.sh | 38 ++++++++++++ scripts/test-consistency.sh | 113 ++++++++++++++++++++++++++++++++++++ 3 files changed, 192 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 191e3c2ea..89f1a4ad1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,49 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BEHAVIOUR CHANGE — `ZADD ... GT LT` and a NaN `WEIGHTS` value now error** + where they previously succeeded (moon#969). `ZADD k GT LT 1 m` used to reply + `(integer) 1` and, on an existing member, `(integer) 0` with the score left + alone; it is now `ERR GT, LT, and/or NX options at the same time are not + compatible`, as on Redis. `ZUNIONSTORE`/`ZINTERSTORE`/`ZUNION`/`ZINTER` with + `WEIGHTS nan` used to be accepted and poison every aggregated score; it is now + `ERR weight value is not a float`. Infinite weights remain legal. A client + relying on either form silently doing nothing will now see an error. + ### Fixed +- **Sorted-set argument validation reports the error CLASS Redis reports** + (moon#969). Nine forms answered the wrong class, which matters beyond wording: + redis-py raises a distinct exception type per class, so a client branching on + the exception took the wrong branch and retried a request that could never + succeed. `ZPOPMIN k notanint`/`k -1` now say `value is out of range, must be + positive`; `ZINTERCARD 0 k` and `ZUNIONSTORE d 0 k` now say `at least 1 input + key is needed for '' command`; `ZMPOP 0 k MIN` says `numkeys should be + greater than 0`; and a short `WEIGHTS` list, a dangling `AGGREGATE`/`LIMIT`/ + `COUNT`, a `numkeys` overrunning the key list, and `ZADD k 1 a 2` are all + `syntax error` rather than arity errors. The set-operation family SPLITS into + two classes exactly as Redis does — not-a-number is the generic integer error, + a number below 1 names the command — while `ZMPOP` does not split, and arity + is checked first so `ZUNION 0` stays an arity error. Also fixed while + reproducing: `ZINTERCARD k LIMIT -1` (`LIMIT can't be negative`), `ZMPOP ... + COUNT 0` (`count should be greater than 0`), and one moon#967 leftover where + `ZUNIONSTORE d 1 k BOGUS` stepped over the unknown token and answered a + different, successful command. Four ZRANGE-family sites the issue also cites + were verified against a redis 8.6.1 oracle to be ALREADY correct and are + deliberately unchanged, with harness rows pinning them. +- **`ZADD ... CH` counts a rescore exactly instead of against an epsilon + window** (moon#792). Both mutation loops decided `changed` with an ABSOLUTE + `f64::EPSILON`, where Redis's `zsetAdd` compares exactly. `f64::EPSILON` is + the gap between 1.0 and the next double — a RELATIVE quantity — so as a fixed + tolerance it swallowed real moves at every magnitude below 1: rescoring + `0.0000000001` to `0.00000000010000001`, six significant figures, replied `0` + while `ZSCORE` showed the new value. The window also disagreed with + `zset_update_existing`, which moves the member on `to_bits()` inequality, so + the write happened and only the tally pretended otherwise. Any client using + `CH` as a did-anything-change signal silently skipped those updates. Fixed on + both the listpack and B+tree arms, which carried separate copies. - **`LMOVE`/`RPOPLPUSH`/`BLPOP` no longer strand 56 B every time they drain a list to empty** (moon#949). `Database::list_pop_front`/`list_pop_back` credited the popped element back to `used_memory` on the non-empty branch but diff --git a/scripts/test-commands.sh b/scripts/test-commands.sh index bf0cda771..1bb6c38ae 100755 --- a/scripts/test-commands.sh +++ b/scripts/test-commands.sh @@ -913,6 +913,44 @@ if should_run "sorted_set"; then assert_match "ZRANGESTORE" ZRANGESTORE {z}:rstore {z}:A 0 -1 assert_match "ZCARD after ZRANGESTORE" ZCARD {z}:rstore assert_moon_ok "ZSCAN" ZSCAN {z}:A 0 + + # moon#969 / moon#792 -- option semantics and error CLASSES. redis-cli + # prints an error reply on STDOUT with rc=0, so assert_match compares the + # text directly. Every form below was previously unrepresented in either + # harness, which is how they drifted. + assert_match "ZADD GT+LT rejected" ZADD z:e1 GT LT 1 m + assert_match "ZADD GT+LT made no key" EXISTS z:e1 + assert_match "ZADD GT+NX rejected" ZADD z:e1 GT NX 1 m + assert_match "ZADD odd tail is syntax" ZADD z:e1 1 a 2 + assert_match "ZADD no pairs is arity" ZADD z:e1 NX + rcli ZADD {z}:w 1 a >/dev/null 2>&1; mcli ZADD {z}:w 1 a >/dev/null 2>&1 + assert_match "ZUNIONSTORE WEIGHTS nan" ZUNIONSTORE {z}:wd 1 {z}:w WEIGHTS nan + assert_match "ZUNION WEIGHTS nan" ZUNION 1 {z}:w WEIGHTS nan + assert_match "ZUNIONSTORE WEIGHTS inf" ZUNIONSTORE {z}:wd 1 {z}:w WEIGHTS inf + assert_match "ZPOPMIN bad count" ZPOPMIN {z}:w notanint + assert_match "ZPOPMIN negative count" ZPOPMIN {z}:w -1 + assert_match "ZUNIONSTORE numkeys 0" ZUNIONSTORE {z}:wd 0 {z}:w + assert_match "ZUNIONSTORE numkeys -1" ZUNIONSTORE {z}:wd -1 {z}:w + assert_match "ZUNIONSTORE numkeys bad" ZUNIONSTORE {z}:wd notanint {z}:w + assert_match "ZINTERCARD numkeys 0" ZINTERCARD 0 {z}:w + assert_match "ZMPOP numkeys 0" ZMPOP 0 {z}:w MIN + assert_match "ZMPOP COUNT 0" ZMPOP 1 {z}:w MIN COUNT 0 + assert_match "ZINTERCARD LIMIT -1" ZINTERCARD 1 {z}:w LIMIT -1 + assert_match "ZUNIONSTORE bare WEIGHTS" ZUNIONSTORE {z}:wd 1 {z}:w WEIGHTS + assert_match "ZUNIONSTORE bare AGGREG" ZUNIONSTORE {z}:wd 1 {z}:w AGGREGATE + assert_match "ZUNIONSTORE numkeys over" ZUNIONSTORE {z}:wd 2 {z}:w + assert_match "ZUNIONSTORE junk token" ZUNIONSTORE {z}:wd 1 {z}:w BOGUS + # Anti-regression: rank-index and LIMIT parses KEEP the generic integer + # error -- Redis reads them with a NULL message. moon#969 calls these + # wrong; the oracle says they are not. + assert_match "ZRANGE rank stays generic" ZRANGE {z}:w notanint 5 + assert_match "ZRANGE LIMIT stays generic" ZRANGEBYSCORE {z}:w 0 5 LIMIT notanint 5 + assert_match "ZRANDMEMBER stays generic" ZRANDMEMBER {z}:w notanint + # moon#792: CH counts an exact rescore; nextafter(1.0) is exactly one + # f64::EPSILON away and used to be reported as no change. + rcli ZADD z:ch 1 m >/dev/null 2>&1; mcli ZADD z:ch 1 m >/dev/null 2>&1 + assert_match "ZADD CH sub-epsilon" ZADD z:ch CH 1.0000000000000002 m + assert_match "ZADD CH moved the score" ZSCORE z:ch m fi # =========================================================================== diff --git a/scripts/test-consistency.sh b/scripts/test-consistency.sh index 1ae1a8559..afd600738 100755 --- a/scripts/test-consistency.sh +++ b/scripts/test-consistency.sh @@ -839,6 +839,119 @@ assert_both "ZADD bad score is an error" ZADD z:enc:bad 1 a 2 b notafloat assert_both "ZADD bad score creates no key" EXISTS z:enc:bad assert_both "ZADD bad score on listpack errors" ZADD z:enc:lp 4 d notafloat e assert_both "ZADD bad score writes no prefix" ZCARD z:enc:lp + +# --------------------------------------------------------------------------- +# moon#969 / moon#792 -- zset option semantics and error CLASSES. +# +# The class matters beyond the wording: redis-py raises a distinct exception +# type per class, so a client branching on it takes the wrong branch. None of +# these forms had a row in either harness, which is why every one of them was +# free to drift. `{z969}` co-locates destination and sources so the rows keep +# comparing the COMMAND, not the shard routing, at --shards > 1. +# --------------------------------------------------------------------------- +# GT, LT and NX are pairwise incompatible. `GT LT` used to be ACCEPTED and then +# silently no-op'd at BOTH mutation sites -- the listpack arm and the B+tree +# arm each carried a `gt && lt => never update` fallthrough. +assert_both "ZADD GT+LT is rejected" ZADD z:969:gtlt GT LT 1 m +assert_both "ZADD GT+LT creates no key" EXISTS z:969:gtlt +assert_both "ZADD GT+NX is rejected" ZADD z:969:gtlt GT NX 1 m +assert_both "ZADD LT+NX is rejected" ZADD z:969:gtlt LT NX 1 m +assert_both "ZADD GT+LT+NX is rejected" ZADD z:969:gtlt GT LT NX 1 m +# ...and on an EXISTING member, on both encodings, the score must not move. +both ZADD z:969:lp 5 m +assert_both "ZADD GT+LT on a listpack member" ZADD z:969:lp GT LT 9 m +assert_both "ZADD GT+LT left the score alone" ZSCORE z:969:lp m +both ZADD z:969:bt 5 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +assert_both "ZADD GT+LT on a bptree member" ZADD z:969:bt GT LT 9 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +assert_both "ZADD GT+LT left the bptree score" ZSCORE z:969:bt aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +# An ODD score/member tail is `syntax error`; NO pairs at all is an ARITY +# error. moon answered the arity error for both. +assert_both "ZADD odd tail is a syntax error" ZADD z:969:odd 1 a 2 +assert_both "ZADD CH with a lone score" ZADD z:969:odd CH 1 +assert_both "ZADD with no pairs is arity" ZADD z:969:odd NX +# A NaN weight: Rust's parse accepts "nan", C's strtod+isnan does not. +# Infinities stay legal on both. +both ZADD {z969}:src 1 a +assert_both "ZUNIONSTORE WEIGHTS nan" ZUNIONSTORE {z969}:d 1 {z969}:src WEIGHTS nan +assert_both "ZINTERSTORE WEIGHTS nan" ZINTERSTORE {z969}:d 1 {z969}:src WEIGHTS nan +assert_both "ZUNION WEIGHTS nan" ZUNION 1 {z969}:src WEIGHTS nan +assert_both "ZINTER WEIGHTS nan" ZINTER 1 {z969}:src WEIGHTS nan +assert_both "ZUNIONSTORE WEIGHTS nan no key" EXISTS {z969}:d +assert_both "ZUNIONSTORE WEIGHTS inf is legal" ZUNIONSTORE {z969}:d 1 {z969}:src WEIGHTS inf +# ZPOPMIN/ZPOPMAX count: `getPositiveLongFromObject`, one message for every +# failure, and 0 is a legal count. +assert_both "ZPOPMIN count not an integer" ZPOPMIN z:969:lp notanint +assert_both "ZPOPMIN negative count" ZPOPMIN z:969:lp -1 +assert_both "ZPOPMAX count not an integer" ZPOPMAX z:969:lp notanint +assert_both "ZPOPMAX negative count" ZPOPMAX z:969:lp -1 +assert_both "ZPOPMIN count 0 is legal" ZPOPMIN z:969:lp 0 +# numkeys SPLITS into two classes for the set-operation family: not-a-number +# is the generic integer error, a number below 1 names the command. +assert_both "ZUNIONSTORE numkeys 0" ZUNIONSTORE {z969}:d 0 {z969}:src +assert_both "ZUNIONSTORE numkeys -1" ZUNIONSTORE {z969}:d -1 {z969}:src +assert_both "ZUNIONSTORE numkeys notanint" ZUNIONSTORE {z969}:d notanint {z969}:src +assert_both "ZINTERSTORE numkeys 0" ZINTERSTORE {z969}:d 0 {z969}:src +assert_both "ZUNION numkeys 0" ZUNION 0 {z969}:src +assert_both "ZINTER numkeys 0" ZINTER 0 {z969}:src +assert_both "ZDIFF numkeys 0" ZDIFF 0 {z969}:src +assert_both "ZINTERCARD numkeys 0" ZINTERCARD 0 {z969}:src +assert_both "ZINTERCARD numkeys notanint" ZINTERCARD notanint {z969}:src +# Arity is checked FIRST, so a form naming no key never reaches those rules. +assert_both "ZUNION numkeys 0 with no key" ZUNION 0 +assert_both "ZINTERCARD numkeys 0 with no key" ZINTERCARD 0 +# ZMPOP does NOT split -- one message for every numkeys failure. +assert_both "ZMPOP numkeys 0" ZMPOP 0 z:969:lp MIN +assert_both "ZMPOP numkeys -1" ZMPOP -1 z:969:lp MIN +assert_both "ZMPOP numkeys notanint" ZMPOP notanint z:969:lp MIN +assert_both "ZMPOP COUNT 0" ZMPOP 1 z:969:lp MIN COUNT 0 +assert_both "ZMPOP COUNT -1" ZMPOP 1 z:969:lp MIN COUNT -1 +assert_both "ZMPOP COUNT notanint" ZMPOP 1 z:969:lp MIN COUNT notanint +assert_both "ZMPOP rejected pops nothing" ZCARD z:969:lp +# ZINTERCARD LIMIT has its own message too. +assert_both "ZINTERCARD LIMIT -1" ZINTERCARD 1 z:969:lp LIMIT -1 +assert_both "ZINTERCARD LIMIT notanint" ZINTERCARD 1 z:969:lp LIMIT notanint +assert_both "ZINTERCARD LIMIT 0 is unbounded" ZINTERCARD 1 z:969:lp LIMIT 0 +# syntax error, NOT an arity error: a short WEIGHTS list, a dangling +# AGGREGATE/LIMIT/COUNT, a numkeys overrunning the key list, and an unknown +# trailing token (the one option loop the moon#967 sweep missed). +assert_both "ZUNIONSTORE dangling WEIGHTS" ZUNIONSTORE {z969}:d 1 {z969}:src WEIGHTS +assert_both "ZUNION dangling WEIGHTS" ZUNION 1 {z969}:src WEIGHTS +assert_both "ZUNIONSTORE dangling AGGREGATE" ZUNIONSTORE {z969}:d 1 {z969}:src AGGREGATE +assert_both "ZUNIONSTORE numkeys overruns" ZUNIONSTORE {z969}:d 2 {z969}:src +assert_both "ZUNION numkeys overruns" ZUNION 2 {z969}:src +assert_both "ZINTERCARD numkeys overruns" ZINTERCARD 2 {z969}:src +assert_both "ZMPOP numkeys overruns" ZMPOP 2 z:969:lp MIN +assert_both "ZUNIONSTORE unknown token" ZUNIONSTORE {z969}:d 1 {z969}:src BOGUS +assert_both "ZINTERCARD dangling LIMIT" ZINTERCARD 1 z:969:lp LIMIT +assert_both "ZMPOP dangling COUNT" ZMPOP 1 z:969:lp MIN COUNT +# ANTI-REGRESSION (moon#969 cites these as wrong; the oracle says they are +# NOT). A ZRANGE rank index and a `LIMIT offset count` are read by Redis with +# `getLongFromObjectOrReply(..., NULL)`, whose message is exactly the generic +# integer error moon already answers. These rows exist so a later reading of +# moon#969 cannot "fix" them into a divergence. +both ZADD z:969:ok 1 a 2 b +assert_both "ZRANGE rank start stays generic" ZRANGE z:969:ok notanint 5 +assert_both "ZRANGE rank stop stays generic" ZRANGE z:969:ok 0 notanint +assert_both "ZRANGE fractional rank is generic" ZRANGE z:969:ok 1.5 2 +assert_both "ZREVRANGE rank stays generic" ZREVRANGE z:969:ok notanint 5 +assert_both "ZRANGE REV LIMIT stays generic" ZRANGE z:969:ok 0 -1 REV LIMIT notanint 5 +assert_both "ZRANGEBYSCORE LIMIT offset" ZRANGEBYSCORE z:969:ok 0 5 LIMIT notanint 5 +assert_both "ZRANGEBYSCORE LIMIT count" ZRANGEBYSCORE z:969:ok 0 5 LIMIT 0 notanint +assert_both "ZREVRANGEBYSCORE LIMIT offset" ZREVRANGEBYSCORE z:969:ok 5 0 LIMIT notanint 5 +assert_both "ZRANDMEMBER count stays generic" ZRANDMEMBER z:969:ok notanint +assert_both "ZRANGESTORE rank stays generic" ZRANGESTORE {z969}:d z:969:ok notanint 5 +# moon#792: CH counts a rescore EXACTLY, as Redis does. `1.0000000000000002` +# is nextafter(1.0), whose distance from 1.0 is exactly f64::EPSILON -- so the +# old `.abs() > f64::EPSILON` window called this real move "unchanged" while +# the stored score really did change, which the ZSCORE row proves. +both ZADD z:792:lp 1 m +assert_both "ZADD CH sub-epsilon (listpack)" ZADD z:792:lp CH 1.0000000000000002 m +assert_both "ZADD CH sub-epsilon moved score" ZSCORE z:792:lp m +assert_both "ZADD CH rewriting the same score" ZADD z:792:lp CH 1.0000000000000002 m +both ZADD z:792:bt 1 bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb +assert_both "ZADD CH sub-epsilon (bptree)" ZADD z:792:bt CH 1.0000000000000002 bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb +assert_both "ZADD CH bptree moved score" ZSCORE z:792:bt bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + # Exactly zset-max-listpack-entries (128) members is STILL a listpack; one # more promotes to a skiplist on both. One ZADD per step, not 129 — each # `both` spawns two redis-cli processes. From 5dacae0409f19e24055d58d4fad54e3cfb6a9cc5 Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 16 Sep 2026 22:48:28 +0700 Subject: [PATCH 4/7] feat(sorted_set): implement the six missing zset commands and ZADD INCR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ZRANGEBYLEX, ZREVRANGEBYLEX, ZREMRANGEBYRANK, ZREMRANGEBYSCORE, ZREMRANGEBYLEX and ZDIFFSTORE answered `ERR unknown command` on moon, and `ZADD ... INCR` — what redis-py's `zadd(..., incr=True)` sends — answered an arity error (moon#959). docs/commands.md advertised ZRANGEBYLEX all along. Every reply, error surface included, was read off a live redis-server 8.6.1 BEFORE the code was written (206 probes, 165 of them diverging on the pre-fix binary a8eb2efc), and the implementation reuses the bound parsers and range helpers ZRANGE already owns rather than growing a second grammar: * ZRANGEBYLEX / ZREVRANGEBYLEX (`sorted_set_lex.rs`): the same shape as `zrangebyscore_readonly` — one implementation behind both spellings, through the shared-borrow view so a listpack survives the read (moon#928), wired into `dispatch`, `dispatch_read` and the read prefilter. Redis's precedence is kept: option loop, then the range grammar (checked BEFORE the key, so a bad bound on a missing key is an error), then the legacy spelling's own `WITHSCORES not supported in combination with BYLEX`. * ZREMRANGEBY{RANK,SCORE,LEX} (`sorted_set_write.rs`): `zrem`'s two-arm shape — a listpack is trimmed in place and never converted, the B+tree arm credits each member and the table shrink, and a drained key is deleted on both arms (which is also what reclaims the container the accessor fabricates for a missing key). Rank normalisation follows `zremrangeGenericCommand` through a new `rank_window` helper: a stop still negative after `len + stop` is NOT clamped, so `-10 -6` removes nothing. (`zrange_by_rank` clamps it and answers `[a]` where redis answers `[]` — a pre-existing ZRANGE divergence reported separately, not changed here.) * ZDIFFSTORE (`sorted_set_store.rs`, the store family moved out of the write half when it crossed the 1500-line rule): a third `SetOp` arm of `zstore_impl`, so it inherits the two-class numkeys split, the overrun rule and the unknown-token rule of moon#969; WEIGHTS/AGGREGATE are refused as `syntax error`, which is what redis does for `SET_OP_DIFF`. Two family-wide corrections rode along: the sources are now looked up BEFORE the options are parsed, so `ZUNIONSTORE d 1 BOGUS` is WRONGTYPE as on redis; and they are read through `get_sorted_set_ref_if_alive`, so reading a listpack source no longer flattens it to a skiplist. * ZADD INCR: `zincrby` is refactored into `zincr_member` taking the NX/XX/GT/LT flags, in `zsetAdd`'s decision order — NX refuses a present member before the sum is formed, a NaN sum errors with nothing written, GT/LT refuse a sum that does not move the right way (zero included), only XX refuses an absent member, and a refusal on a key the call had to fabricate leaves nothing behind. Reply is the new score as a bulk string, or nil. Pair count is checked after the parity and flag-pairing rules, as redis does. Registered in the phf table as @sortedset with redis's arities; the `used_memory` ledger is asserted exact against a full recount on both encodings for every new write; the cold-tier read test gains the two lex reads. `try_inline_dispatch` inlines only GET and a plain SET, so there is no third arm for these to be missing from. Refs moon#959. Stacked on #991 (moon#969), whose error conventions the new commands follow. author: Tin Dang --- src/command/metadata.rs | 14 + src/command/mod.rs | 37 + src/command/sorted_set/mod.rs | 1011 ++++++++++++++++++++ src/command/sorted_set/sorted_set_lex.rs | 157 +++ src/command/sorted_set/sorted_set_store.rs | 465 +++++++++ src/command/sorted_set/sorted_set_write.rs | 822 ++++++++-------- tests/zset_read_cold_tier_928.rs | 6 + 7 files changed, 2072 insertions(+), 440 deletions(-) create mode 100644 src/command/sorted_set/sorted_set_lex.rs create mode 100644 src/command/sorted_set/sorted_set_store.rs diff --git a/src/command/metadata.rs b/src/command/metadata.rs index 8b5ec5f39..0262a25a0 100644 --- a/src/command/metadata.rs +++ b/src/command/metadata.rs @@ -309,6 +309,14 @@ pub static COMMAND_META: phf::Map<&'static str, CommandMeta> = phf_map! { "ZMSCORE" => CommandMeta { name: "ZMSCORE", arity: -3, flags: RFP, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, "ZRANDMEMBER" => CommandMeta { name: "ZRANDMEMBER", arity: -2, flags: R, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, "ZMPOP" => CommandMeta { name: "ZMPOP", arity: -4, flags: W, first_key: 0, last_key: 0, step: 0, acl_categories: ZST }, + // moon#959. Arities and key specs transcribed from redis 8.6.1's + // `COMMAND INFO`; the replies themselves were verified on the wire. + "ZRANGEBYLEX" => CommandMeta { name: "ZRANGEBYLEX", arity: -4, flags: R, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, + "ZREVRANGEBYLEX" => CommandMeta { name: "ZREVRANGEBYLEX", arity: -4, flags: R, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, + "ZREMRANGEBYRANK" => CommandMeta { name: "ZREMRANGEBYRANK", arity: 4, flags: W, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, + "ZREMRANGEBYSCORE" => CommandMeta { name: "ZREMRANGEBYSCORE", arity: 4, flags: W, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, + "ZREMRANGEBYLEX" => CommandMeta { name: "ZREMRANGEBYLEX", arity: 4, flags: W, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, + "ZDIFFSTORE" => CommandMeta { name: "ZDIFFSTORE", arity: -4, flags: W, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, // ---- Stream commands ---- "XADD" => CommandMeta { name: "XADD", arity: -5, flags: WF, first_key: 1, last_key: 1, step: 1, acl_categories: STM }, @@ -1542,6 +1550,10 @@ mod tests { b"ZINTERSTORE", b"ZRANGESTORE", b"ZMPOP", + b"ZREMRANGEBYRANK", + b"ZREMRANGEBYSCORE", + b"ZREMRANGEBYLEX", + b"ZDIFFSTORE", b"HINCRBYFLOAT", b"LSET", b"LREM", @@ -1579,6 +1591,8 @@ mod tests { b"SMEMBERS", b"SISMEMBER", b"ZRANGEBYSCORE", + b"ZRANGEBYLEX", + b"ZREVRANGEBYLEX", b"BITFIELD_RO", b"SORT_RO", b"GEORADIUS_RO", diff --git a/src/command/mod.rs b/src/command/mod.rs index 54c3b55bc..a11cae0a4 100644 --- a/src/command/mod.rs +++ b/src/command/mod.rs @@ -996,6 +996,9 @@ fn dispatch_inner( if cmd.eq_ignore_ascii_case(b"ZINTERCARD") { return resp(sorted_set::zintercard(db, args)); } + if cmd.eq_ignore_ascii_case(b"ZDIFFSTORE") { + return resp(sorted_set::zdiffstore(db, args)); + } } // 11-letter commands (11, b'p') => { @@ -1042,6 +1045,9 @@ fn dispatch_inner( if cmd.eq_ignore_ascii_case(b"ZRANDMEMBER") { return resp(sorted_set::zrandmember(db, args)); } + if cmd.eq_ignore_ascii_case(b"ZRANGEBYLEX") { + return resp(sorted_set::zrangebylex(db, args)); + } } // 11-letter commands (hash) (11, b'h') => { @@ -1074,6 +1080,15 @@ fn dispatch_inner( } } // 14-letter commands + (14, b'z') => { + // ZREVRANGEBYLEX ZREMRANGEBYLEX + if cmd.eq_ignore_ascii_case(b"ZREVRANGEBYLEX") { + return resp(sorted_set::zrevrangebylex(db, args)); + } + if cmd.eq_ignore_ascii_case(b"ZREMRANGEBYLEX") { + return resp(sorted_set::zremrangebylex(db, args)); + } + } (14, b'g') => { // GEOSEARCHSTORE if cmd.eq_ignore_ascii_case(b"GEOSEARCHSTORE") { @@ -1086,6 +1101,16 @@ fn dispatch_inner( if cmd.eq_ignore_ascii_case(b"ZREVRANGEBYSCORE") { return resp(sorted_set::zrevrangebyscore(db, args)); } + if cmd.eq_ignore_ascii_case(b"ZREMRANGEBYSCORE") { + return resp(sorted_set::zremrangebyscore(db, args)); + } + } + // 15-letter commands + (15, b'z') => { + // ZREMRANGEBYRANK + if cmd.eq_ignore_ascii_case(b"ZREMRANGEBYRANK") { + return resp(sorted_set::zremrangebyrank(db, args)); + } } // 17-letter commands (17, b'g') => { @@ -1208,6 +1233,7 @@ pub fn is_dispatch_read_supported(cmd: &[u8]) -> bool { | (12, b'h') // HPEXPIRETIME | (12, b'g') // GEORADIUS_RO | (13, b'z') // ZRANGEBYSCORE + | (14, b'z') // ZREVRANGEBYLEX | (16, b'z') // ZREVRANGEBYSCORE | (20, b'g') // GEORADIUSBYMEMBER_RO ) @@ -1615,6 +1641,12 @@ fn dispatch_read_inner(db: &Database, cmd: &[u8], args: &[Frame], now_ms: u64) - return resp(sorted_set::zrevrangebyscore_readonly(db, args, now_ms)); } } + (14, b'z') => { + // ZREVRANGEBYLEX + if cmd.eq_ignore_ascii_case(b"ZREVRANGEBYLEX") { + return resp(sorted_set::zrevrangebylex_readonly(db, args, now_ms)); + } + } // ---- new arms (contract v2): buckets that don't conflict with pre-existing ones ---- (3, b'l') => { // LCS @@ -1728,6 +1760,9 @@ fn dispatch_read_inner(db: &Database, cmd: &[u8], args: &[Frame], now_ms: u64) - if cmd.eq_ignore_ascii_case(b"ZRANDMEMBER") { return resp(sorted_set::zrandmember_readonly(db, args, now_ms)); } + if cmd.eq_ignore_ascii_case(b"ZRANGEBYLEX") { + return resp(sorted_set::zrangebylex_readonly(db, args, now_ms)); + } } (11, b'b') => { // BITFIELD_RO (11 bytes) @@ -2360,6 +2395,8 @@ mod tests { b"ZLEXCOUNT", b"ZRANGEBYSCORE", b"ZREVRANGEBYSCORE", + b"ZRANGEBYLEX", + b"ZREVRANGEBYLEX", b"LLEN", b"LRANGE", b"LINDEX", diff --git a/src/command/sorted_set/mod.rs b/src/command/sorted_set/mod.rs index a241ed533..fc4d703f7 100644 --- a/src/command/sorted_set/mod.rs +++ b/src/command/sorted_set/mod.rs @@ -1,8 +1,12 @@ +mod sorted_set_lex; mod sorted_set_read; +mod sorted_set_store; mod sorted_set_write; mod work_budget; +pub use sorted_set_lex::*; pub use sorted_set_read::*; +pub use sorted_set_store::*; pub use sorted_set_write::*; use bytes::Bytes; @@ -371,6 +375,43 @@ pub(super) fn lex_in_range(member: &[u8], min: &LexBound, max: &LexBound) -> boo // Shared range helpers // --------------------------------------------------------------------------- +/// Resolve a `start stop` rank pair the way Redis's `zremrangeGenericCommand` +/// does, returning the inclusive window or `None` when it is empty. +/// +/// A negative index counts from the end. A START still negative after that is +/// clamped to 0; a STOP still negative is NOT, so `start > stop` reports the +/// window empty — which is what makes `ZREMRANGEBYRANK z -10 -6` on a +/// five-member zset remove nothing (redis 8.6.1 answers `(integer) 0`). +/// +/// `zrange_by_rank` and `zrange_from_entries` below clamp the STOP as well and +/// answer `[a]` for the same `ZRANGE z -10 -6`, where redis answers `[]`. That +/// is a pre-existing divergence in a command moon#959 does not touch; it is +/// reported separately rather than changed under this issue, and this helper +/// exists so the new command does not inherit it. +pub(super) fn rank_window(start_raw: i64, stop_raw: i64, total: usize) -> Option<(usize, usize)> { + let len = total as i64; + let mut start = if start_raw < 0 { + len.saturating_add(start_raw) + } else { + start_raw + }; + let mut stop = if stop_raw < 0 { + len.saturating_add(stop_raw) + } else { + stop_raw + }; + if start < 0 { + start = 0; + } + if start > stop || start >= len { + return None; + } + if stop >= len { + stop = len - 1; + } + Some((start as usize, stop as usize)) +} + pub(super) fn zrange_by_rank( scores: &BPTree, min_arg: &[u8], @@ -3984,3 +4025,973 @@ mod zadd_listpack_batch_tests { ); } } + +/// moon#959 — the six commands that used to be `unknown command`, and +/// `ZADD ... INCR`. Every expectation below was read off redis-server 8.6.1 +/// on the wire (`/tmp/z959/oracle_vs_control.txt` in the PR) before the code +/// was written; the reply bytes are what these assert, not `COMMAND INFO`. +/// +/// Dispatch-path coverage, stated per CLAUDE.md's three-path rule: +/// * `command::dispatch` (the mutable path MULTI/EXEC, Lua and every write +/// take) — every `call(...)` below goes through it. +/// * `command::dispatch_read` (the shared-lock path a bare read takes) — +/// `call_read(...)` for the two lex reads, plus the prefilter check in +/// `dispatch_read_serves_the_lex_reads`. +/// * `server::conn::try_inline_dispatch` inlines exactly `GET` and a plain +/// `SET` (`blocking.rs`); every other command falls through to generic +/// dispatch, so there is no arm for a zset command to be missing from. +#[cfg(test)] +mod missing_commands_959_tests { + use super::*; + use crate::command::{DispatchResult, dispatch, dispatch_read, is_dispatch_read_supported}; + use crate::storage::Database; + + fn bs(s: &[u8]) -> Frame { + Frame::BulkString(Bytes::copy_from_slice(s)) + } + + fn argv(args: &[&str]) -> Vec { + args.iter().map(|a| bs(a.as_bytes())).collect() + } + + /// Through the real mutable dispatch table, so a missing arm shows up as + /// `unknown command` rather than as a handler that was never reached. + fn call(db: &mut Database, cmd: &str, args: &[&str]) -> Frame { + let mut selected = 0usize; + match dispatch(db, cmd.as_bytes(), &argv(args), &mut selected, 16) { + DispatchResult::Response(f) => f, + DispatchResult::Quit(f) => panic!("unexpected Quit for {cmd}: {f:?}"), + } + } + + /// Through the shared-lock read table. + fn call_read(db: &Database, cmd: &str, args: &[&str]) -> Frame { + let mut selected = 0usize; + let now_ms = db.now_ms(); + match dispatch_read(db, cmd.as_bytes(), &argv(args), now_ms, &mut selected, 16) { + DispatchResult::Response(f) => f, + DispatchResult::Quit(f) => panic!("unexpected Quit for {cmd}: {f:?}"), + } + } + + fn seed(db: &mut Database, key: &str, pairs: &[(&str, &str)]) { + let mut a = vec![key]; + for (s, m) in pairs { + a.push(s); + a.push(m); + } + let n = call(db, "ZADD", &a); + assert_eq!(n, Frame::Integer(pairs.len() as i64), "seeding {key}"); + } + + const FIVE: &[(&str, &str)] = &[("1", "a"), ("2", "b"), ("3", "c"), ("4", "d"), ("5", "e")]; + const LEX: &[(&str, &str)] = &[("0", "a"), ("0", "b"), ("0", "c"), ("0", "d"), ("0", "e")]; + + /// A member past `zset-max-listpack-value` (64) forces the B+tree form. + /// All `z`s so it sorts AFTER every fixture member under a lex range. + const LONG: &str = "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"; + + fn strings(frame: &Frame) -> Vec { + match frame { + Frame::Array(items) => items + .iter() + .map(|f| match f { + Frame::BulkString(b) => String::from_utf8_lossy(b).into_owned(), + other => panic!("not a bulk string: {other:?}"), + }) + .collect(), + other => panic!("not an array: {other:?}"), + } + } + + fn range(db: &mut Database, key: &str) -> Vec { + strings(&call(db, "ZRANGE", &[key, "0", "-1"])) + } + + fn err_text(frame: &Frame) -> String { + match frame { + Frame::Error(e) => String::from_utf8_lossy(e).into_owned(), + other => panic!("expected an error reply, got {other:?}"), + } + } + + fn encoding_of(db: &mut Database, key: &str) -> String { + match crate::command::key::object(db, &[bs(b"ENCODING"), bs(key.as_bytes())]) { + Frame::BulkString(b) => String::from_utf8_lossy(&b).into_owned(), + other => panic!("OBJECT ENCODING did not reply a bulk string: {other:?}"), + } + } + + fn exists(db: &mut Database, key: &str) -> bool { + call(db, "EXISTS", &[key]) == Frame::Integer(1) + } + + fn ledger_exact(db: &mut Database, step: &str) { + let running = db.estimated_memory(); + db.recalculate_memory(); + let recomputed = db.estimated_memory(); + assert_eq!( + running, recomputed, + "{step}: ledger {running} != recount {recomputed}" + ); + } + + // ── the tripwire: nothing here is `unknown command` any more ──────── + + #[test] + fn all_six_are_dispatched_and_the_negative_control_is_not() { + let mut db = Database::new(); + seed(&mut db, "z", FIVE); + for (cmd, args) in [ + ("ZRANGEBYLEX", &["z", "-", "+"][..]), + ("ZREVRANGEBYLEX", &["z", "+", "-"][..]), + ("ZREMRANGEBYRANK", &["z", "0", "0"][..]), + ("ZREMRANGEBYSCORE", &["z", "0", "0"][..]), + ("ZREMRANGEBYLEX", &["z", "[zz", "[zz"][..]), + ("ZDIFFSTORE", &["d", "1", "z"][..]), + ] { + let reply = call(&mut db, cmd, args); + assert!( + !matches!(&reply, Frame::Error(e) if e.starts_with(b"ERR unknown command")), + "{cmd} is still unknown to dispatch: {reply:?}" + ); + // Lower-case, as redis-py sends it. + let reply = call(&mut db, &cmd.to_ascii_lowercase(), args); + assert!( + !matches!(&reply, Frame::Error(e) if e.starts_with(b"ERR unknown command")), + "{cmd} (lower-case) is still unknown to dispatch: {reply:?}" + ); + } + // The negative control: the same shape the issue used, still refused. + let reply = call(&mut db, "ZNOTACOMMAND", &["z"]); + assert!( + err_text(&reply).starts_with("ERR unknown command"), + "{reply:?}" + ); + } + + #[test] + fn registry_carries_the_six_with_the_sortedset_category() { + use crate::command::metadata::{AclCategories, CommandFlags, lookup}; + for (name, write, arity) in [ + ("ZRANGEBYLEX", false, -4), + ("ZREVRANGEBYLEX", false, -4), + ("ZREMRANGEBYRANK", true, 4), + ("ZREMRANGEBYSCORE", true, 4), + ("ZREMRANGEBYLEX", true, 4), + ("ZDIFFSTORE", true, -4), + ] { + let meta = lookup(name.as_bytes()).unwrap_or_else(|| panic!("{name} not registered")); + assert_eq!(meta.arity, arity, "{name} arity"); + assert_eq!( + meta.flags.contains(CommandFlags::WRITE), + write, + "{name} write flag" + ); + assert_eq!( + meta.flags.contains(CommandFlags::READONLY), + !write, + "{name} read flag" + ); + assert!( + meta.acl_categories.contains(AclCategories::SORTEDSET), + "{name} must be @sortedset" + ); + assert_eq!(meta.first_key, 1, "{name} first key"); + } + } + + #[test] + fn dispatch_read_serves_the_lex_reads() { + let mut db = Database::new(); + seed(&mut db, "lex", LEX); + assert!(is_dispatch_read_supported(b"ZRANGEBYLEX")); + assert!(is_dispatch_read_supported(b"ZREVRANGEBYLEX")); + assert_eq!( + strings(&call_read(&db, "ZRANGEBYLEX", &["lex", "[b", "(d"])), + ["b", "c"] + ); + assert_eq!( + strings(&call_read(&db, "ZREVRANGEBYLEX", &["lex", "(d", "[b"])), + ["c", "b"] + ); + // The read path answered from the listpack without flattening it. + assert_eq!(encoding_of(&mut db, "lex"), "listpack"); + } + + // ── ZRANGEBYLEX / ZREVRANGEBYLEX ───────────────────────────────────── + + #[test] + fn zrangebylex_bounds_and_limit_match_the_oracle() { + for promote in [false, true] { + let mut db = Database::new(); + seed(&mut db, "lex", LEX); + if promote { + call(&mut db, "ZADD", &["lex", "0", LONG]); + assert_eq!(encoding_of(&mut db, "lex"), "skiplist"); + } + let r = |db: &mut Database, a: &[&str]| strings(&call(db, "ZRANGEBYLEX", a)); + let tail: &[&str] = if promote { &[LONG] } else { &[] }; + let mut all = vec!["a", "b", "c", "d", "e"]; + all.extend_from_slice(tail); + assert_eq!(r(&mut db, &["lex", "-", "+"]), all, "promote={promote}"); + assert_eq!(r(&mut db, &["lex", "[b", "(d"]), ["b", "c"]); + assert_eq!(r(&mut db, &["lex", "(b", "[d"]), ["c", "d"]); + assert_eq!(r(&mut db, &["lex", "[c", "[c"]), ["c"]); + // `(` and `[` alone are exclusive/inclusive EMPTY strings: every + // member is > "" and none is <= "". + assert!(r(&mut db, &["lex", "(", "["]).is_empty()); + assert_eq!( + r(&mut db, &["lex", "-", "+", "LIMIT", "1", "2"]), + ["b", "c"] + ); + assert!(r(&mut db, &["lex", "-", "+", "LIMIT", "-1", "2"]).is_empty()); + assert!(r(&mut db, &["lex", "-", "+", "LIMIT", "0", "0"]).is_empty()); + let mut from_b = vec!["b", "c", "d", "e"]; + from_b.extend_from_slice(tail); + assert_eq!(r(&mut db, &["lex", "-", "+", "LIMIT", "1", "-1"]), from_b); + // Reversed bounds are an empty range, not an error. + assert!(r(&mut db, &["lex", "+", "-"]).is_empty()); + // Lower-case option token. + assert_eq!( + r(&mut db, &["lex", "-", "+", "limit", "1", "2"]), + ["b", "c"] + ); + assert!(r(&mut db, &["nokey", "-", "+"]).is_empty()); + } + } + + #[test] + fn zrevrangebylex_takes_max_then_min_and_walks_backwards() { + let mut db = Database::new(); + seed(&mut db, "lex", LEX); + let r = |db: &mut Database, a: &[&str]| strings(&call(db, "ZREVRANGEBYLEX", a)); + assert_eq!(r(&mut db, &["lex", "+", "-"]), ["e", "d", "c", "b", "a"]); + assert_eq!(r(&mut db, &["lex", "[d", "(b"]), ["d", "c"]); + assert_eq!(r(&mut db, &["lex", "(d", "[b"]), ["c", "b"]); + assert!(r(&mut db, &["lex", "-", "+"]).is_empty()); + assert_eq!( + r(&mut db, &["lex", "+", "-", "LIMIT", "1", "2"]), + ["d", "c"] + ); + assert_eq!( + r(&mut db, &["lex", "+", "-", "LIMIT", "1", "-1"]), + ["d", "c", "b", "a"] + ); + assert!(r(&mut db, &["lex", "+", "-", "LIMIT", "-1", "2"]).is_empty()); + } + + #[test] + fn zrangebylex_error_surface_matches_the_oracle() { + let mut db = Database::new(); + seed(&mut db, "lex", LEX); + call(&mut db, "SET", &["str", "v"]); + let e = |db: &mut Database, cmd: &str, a: &[&str]| err_text(&call(db, cmd, a)); + for cmd in ["ZRANGEBYLEX", "ZREVRANGEBYLEX"] { + let lc = cmd.to_ascii_lowercase(); + assert_eq!( + e(&mut db, cmd, &["lex", "-"]), + format!("ERR wrong number of arguments for '{lc}' command") + ); + assert_eq!( + e(&mut db, cmd, &["lex", "a", "b"]), + "ERR min or max not valid string range item" + ); + assert_eq!( + e(&mut db, cmd, &["lex", "", "+"]), + "ERR min or max not valid string range item" + ); + // The grammar is checked BEFORE the key: a missing key with a bad + // bound is still an error, not an empty array. + assert_eq!( + e(&mut db, cmd, &["nokey", "a", "b"]), + "ERR min or max not valid string range item" + ); + assert_eq!( + e(&mut db, cmd, &["lex", "-", "+", "WITHSCORES"]), + "ERR syntax error, WITHSCORES not supported in combination with BYLEX" + ); + // ... but a bad bound outranks the WITHSCORES refusal. + assert_eq!( + e(&mut db, cmd, &["lex", "a", "b", "WITHSCORES"]), + "ERR min or max not valid string range item" + ); + assert_eq!( + e(&mut db, cmd, &["lex", "-", "+", "LIMIT", "1"]), + "ERR syntax error" + ); + assert_eq!( + e(&mut db, cmd, &["lex", "-", "+", "BOGUS"]), + "ERR syntax error" + ); + assert_eq!( + e(&mut db, cmd, &["lex", "-", "+", "LIMIT", "notanint", "1"]), + "ERR value is not an integer or out of range" + ); + assert_eq!( + e(&mut db, cmd, &["lex", "-", "+", "LIMIT", "1", "notanint"]), + "ERR value is not an integer or out of range" + ); + // The option loop runs first: a dangling LIMIT beats a bad bound. + assert_eq!( + e(&mut db, cmd, &["lex", "a", "b", "LIMIT", "1"]), + "ERR syntax error" + ); + assert!(e(&mut db, cmd, &["str", "-", "+"]).starts_with("WRONGTYPE")); + } + } + + // ── ZREMRANGEBYRANK ────────────────────────────────────────────────── + + #[test] + fn zremrangebyrank_normalises_ranks_like_redis() { + for promote in [false, true] { + let cases: &[(&str, &str, i64, &[&str])] = &[ + ("0", "0", 1, &["b", "c", "d", "e"]), + ("-2", "-1", 2, &["a", "b", "c"]), + ("3", "1", 0, &["a", "b", "c", "d", "e"]), + ("0", "100", 5, &[]), + ("-100", "1", 2, &["c", "d", "e"]), + ("5", "10", 0, &["a", "b", "c", "d", "e"]), + ("-1", "-3", 0, &["a", "b", "c", "d", "e"]), + // A stop still negative after normalisation is NOT clamped + // to 0: redis 8.6.1 removes nothing here. + ("-10", "-6", 0, &["a", "b", "c", "d", "e"]), + ("2", "-2", 2, &["a", "b", "e"]), + ("0", "-1", 5, &[]), + ]; + for (start, stop, removed, left) in cases { + let mut db = Database::new(); + seed(&mut db, "r", FIVE); + if promote { + // Promote WITHOUT changing the membership under test. + call(&mut db, "ZADD", &["r", "9", LONG]); + call(&mut db, "ZREM", &["r", LONG]); + assert_eq!(encoding_of(&mut db, "r"), "skiplist"); + } else { + assert_eq!(encoding_of(&mut db, "r"), "listpack"); + } + assert_eq!( + call(&mut db, "ZREMRANGEBYRANK", &["r", start, stop]), + Frame::Integer(*removed), + "ZREMRANGEBYRANK r {start} {stop} promote={promote}" + ); + if left.is_empty() { + assert!(!exists(&mut db, "r"), "drained key must be gone"); + } else { + assert_eq!( + range(&mut db, "r"), + *left, + "{start} {stop} promote={promote}" + ); + // A removal never converts the encoding (moon#897). + assert_eq!( + encoding_of(&mut db, "r"), + if promote { "skiplist" } else { "listpack" } + ); + } + ledger_exact( + &mut db, + &format!("ZREMRANGEBYRANK {start} {stop} promote={promote}"), + ); + } + } + } + + #[test] + fn zremrangebyrank_error_surface_and_missing_key() { + let mut db = Database::new(); + seed(&mut db, "r", FIVE); + call(&mut db, "SET", &["str", "v"]); + let e = |db: &mut Database, a: &[&str]| err_text(&call(db, "ZREMRANGEBYRANK", a)); + for bad in [ + &["r", "notanint", "1"][..], + &["r", "1", "notanint"], + &["r", "1.5", "2"], + ] { + assert_eq!( + e(&mut db, bad), + "ERR value is not an integer or out of range" + ); + } + for bad in [&["r", "1"][..], &["r", "1", "2", "3"], &["r"]] { + assert_eq!( + e(&mut db, bad), + "ERR wrong number of arguments for 'zremrangebyrank' command" + ); + } + assert!(e(&mut db, &["str", "0", "1"]).starts_with("WRONGTYPE")); + assert_eq!( + range(&mut db, "r").len(), + 5, + "no error may have removed anything" + ); + // A bad index on a MISSING key is still the integer error (the range + // is parsed before the lookup), and a good one answers 0 and creates + // nothing. + assert_eq!( + e(&mut db, &["nokey", "x", "1"]), + "ERR value is not an integer or out of range" + ); + assert_eq!( + call(&mut db, "ZREMRANGEBYRANK", &["nokey", "0", "1"]), + Frame::Integer(0) + ); + assert!(!exists(&mut db, "nokey")); + ledger_exact(&mut db, "after the error surface"); + } + + // ── ZREMRANGEBYSCORE ───────────────────────────────────────────────── + + #[test] + fn zremrangebyscore_bounds_match_the_oracle() { + for promote in [false, true] { + let cases: &[(&str, &str, i64, &[&str])] = &[ + ("2", "3", 2, &["a", "d", "e"]), + ("(2", "3", 1, &["a", "b", "d", "e"]), + ("-inf", "+inf", 5, &[]), + ("3", "1", 0, &["a", "b", "c", "d", "e"]), + ("+inf", "-inf", 0, &["a", "b", "c", "d", "e"]), + ("(1", "(1", 0, &["a", "b", "c", "d", "e"]), + ("(1", "2", 1, &["a", "c", "d", "e"]), + ("(5", "inf", 0, &["a", "b", "c", "d", "e"]), + ("5", "inf", 1, &["a", "b", "c", "d"]), + ]; + for (min, max, removed, left) in cases { + let mut db = Database::new(); + seed(&mut db, "s", FIVE); + if promote { + call(&mut db, "ZADD", &["s", "9", LONG]); + call(&mut db, "ZREM", &["s", LONG]); + assert_eq!(encoding_of(&mut db, "s"), "skiplist"); + } + assert_eq!( + call(&mut db, "ZREMRANGEBYSCORE", &["s", min, max]), + Frame::Integer(*removed), + "ZREMRANGEBYSCORE s {min} {max} promote={promote}" + ); + if left.is_empty() { + assert!(!exists(&mut db, "s")); + } else { + assert_eq!(range(&mut db, "s"), *left, "{min} {max} promote={promote}"); + } + ledger_exact( + &mut db, + &format!("ZREMRANGEBYSCORE {min} {max} promote={promote}"), + ); + } + } + } + + #[test] + fn zremrangebyscore_error_surface_and_missing_key() { + let mut db = Database::new(); + seed(&mut db, "s", FIVE); + call(&mut db, "SET", &["str", "v"]); + let e = |db: &mut Database, a: &[&str]| err_text(&call(db, "ZREMRANGEBYSCORE", a)); + for bad in [&["s", "nan", "1"][..], &["s", "a", "1"], &["s", "1", "a"]] { + assert_eq!(e(&mut db, bad), "ERR min or max is not a float"); + } + for bad in [&["s", "1"][..], &["s", "1", "2", "3"]] { + assert_eq!( + e(&mut db, bad), + "ERR wrong number of arguments for 'zremrangebyscore' command" + ); + } + assert!(e(&mut db, &["str", "0", "1"]).starts_with("WRONGTYPE")); + assert_eq!( + e(&mut db, &["nokey", "x", "1"]), + "ERR min or max is not a float" + ); + assert_eq!( + call(&mut db, "ZREMRANGEBYSCORE", &["nokey", "0", "1"]), + Frame::Integer(0) + ); + assert!(!exists(&mut db, "nokey")); + assert_eq!(range(&mut db, "s").len(), 5); + } + + // ── ZREMRANGEBYLEX ─────────────────────────────────────────────────── + + #[test] + fn zremrangebylex_bounds_match_the_oracle() { + for promote in [false, true] { + let cases: &[(&str, &str, i64, &[&str])] = &[ + ("[b", "(d", 2, &["a", "d", "e"]), + ("-", "+", 5, &[]), + ("+", "-", 0, &["a", "b", "c", "d", "e"]), + ("(c", "+", 2, &["a", "b", "c"]), + ("[zz", "[zz", 0, &["a", "b", "c", "d", "e"]), + ]; + for (min, max, removed, left) in cases { + let mut db = Database::new(); + seed(&mut db, "l", LEX); + if promote { + call(&mut db, "ZADD", &["l", "0", LONG]); + call(&mut db, "ZREM", &["l", LONG]); + assert_eq!(encoding_of(&mut db, "l"), "skiplist"); + } + assert_eq!( + call(&mut db, "ZREMRANGEBYLEX", &["l", min, max]), + Frame::Integer(*removed), + "ZREMRANGEBYLEX l {min} {max} promote={promote}" + ); + if left.is_empty() { + assert!(!exists(&mut db, "l")); + } else { + assert_eq!(range(&mut db, "l"), *left, "{min} {max} promote={promote}"); + } + ledger_exact( + &mut db, + &format!("ZREMRANGEBYLEX {min} {max} promote={promote}"), + ); + } + } + } + + #[test] + fn zremrangebylex_error_surface_and_missing_key() { + let mut db = Database::new(); + seed(&mut db, "l", LEX); + call(&mut db, "SET", &["str", "v"]); + let e = |db: &mut Database, a: &[&str]| err_text(&call(db, "ZREMRANGEBYLEX", a)); + assert_eq!( + e(&mut db, &["l", "a", "b"]), + "ERR min or max not valid string range item" + ); + assert_eq!( + e(&mut db, &["l", "", "+"]), + "ERR min or max not valid string range item" + ); + for bad in [&["l", "-"][..], &["l", "-", "+", "x"]] { + assert_eq!( + e(&mut db, bad), + "ERR wrong number of arguments for 'zremrangebylex' command" + ); + } + assert!(e(&mut db, &["str", "-", "+"]).starts_with("WRONGTYPE")); + assert_eq!( + e(&mut db, &["nokey", "x", "1"]), + "ERR min or max not valid string range item" + ); + assert_eq!( + call(&mut db, "ZREMRANGEBYLEX", &["nokey", "-", "+"]), + Frame::Integer(0) + ); + assert!(!exists(&mut db, "nokey")); + assert_eq!(range(&mut db, "l").len(), 5); + } + + // ── ZDIFFSTORE ─────────────────────────────────────────────────────── + + fn scored(db: &mut Database, key: &str) -> Vec { + strings(&call(db, "ZRANGE", &[key, "0", "-1", "WITHSCORES"])) + } + + #[test] + fn zdiffstore_computes_the_difference_with_first_source_scores() { + let mut db = Database::new(); + seed(&mut db, "z", FIVE); + seed(&mut db, "z2", &[("1", "a"), ("2", "b")]); + seed(&mut db, "z3", &[("2", "b"), ("9", "x")]); + assert_eq!( + call(&mut db, "ZDIFFSTORE", &["d1", "2", "z", "z2"]), + Frame::Integer(3) + ); + assert_eq!(scored(&mut db, "d1"), ["c", "3", "d", "4", "e", "5"]); + assert_eq!( + call(&mut db, "ZDIFFSTORE", &["d2", "1", "z"]), + Frame::Integer(5) + ); + assert_eq!( + call(&mut db, "ZDIFFSTORE", &["d3", "2", "z", "nokey"]), + Frame::Integer(5) + ); + assert_eq!( + call(&mut db, "ZDIFFSTORE", &["d8", "3", "z", "z2", "z3"]), + Frame::Integer(3) + ); + assert_eq!(scored(&mut db, "d8"), ["c", "3", "d", "4", "e", "5"]); + // Sources are read before the destination is replaced, so a + // destination that is also a source is diffed from its OLD content. + assert_eq!( + call(&mut db, "ZDIFFSTORE", &["z3", "2", "z", "z3"]), + Frame::Integer(4) + ); + assert_eq!( + scored(&mut db, "z3"), + ["a", "1", "c", "3", "d", "4", "e", "5"] + ); + assert_eq!( + call(&mut db, "ZDIFFSTORE", &["z2", "1", "z2"]), + Frame::Integer(2) + ); + assert_eq!(scored(&mut db, "z2"), ["a", "1", "b", "2"]); + // Lower-case, as a client library sends it. + assert_eq!( + call(&mut db, "zdiffstore", &["d9", "1", "z"]), + Frame::Integer(5) + ); + // Reading a listpack source did not flatten it. + assert_eq!(encoding_of(&mut db, "z"), "listpack"); + ledger_exact(&mut db, "after the ZDIFFSTORE happy paths"); + } + + #[test] + fn zdiffstore_empty_result_deletes_the_destination() { + let mut db = Database::new(); + seed(&mut db, "z", FIVE); + assert_eq!( + call(&mut db, "ZDIFFSTORE", &["d4", "2", "nokey", "z"]), + Frame::Integer(0) + ); + assert!(!exists(&mut db, "d4")); + // Even a destination of another type is replaced — by nothing. + call(&mut db, "SET", &["d5", "x"]); + assert_eq!( + call(&mut db, "ZDIFFSTORE", &["d5", "2", "z", "z"]), + Frame::Integer(0) + ); + assert!(!exists(&mut db, "d5")); + ledger_exact(&mut db, "after an empty ZDIFFSTORE"); + } + + #[test] + fn zdiffstore_error_surface_matches_the_oracle() { + let mut db = Database::new(); + seed(&mut db, "z", FIVE); + call(&mut db, "SET", &["str", "v"]); + let e = |db: &mut Database, a: &[&str]| err_text(&call(db, "ZDIFFSTORE", a)); + // The two-class numkeys split (moon#969). + assert_eq!( + e(&mut db, &["d", "0", "z"]), + "ERR at least 1 input key is needed for 'zdiffstore' command" + ); + assert_eq!( + e(&mut db, &["d", "-1", "z"]), + "ERR at least 1 input key is needed for 'zdiffstore' command" + ); + assert_eq!( + e(&mut db, &["d", "notanint", "z"]), + "ERR value is not an integer or out of range" + ); + // Arity first: no key named at all. + for bad in [&["d", "1"][..], &["d"], &["d", "0"]] { + assert_eq!( + e(&mut db, bad), + "ERR wrong number of arguments for 'zdiffstore' command" + ); + } + // numkeys overrunning the key list, and every option token: ZDIFFSTORE + // takes none, so WEIGHTS/AGGREGATE are as unknown as BOGUS. + assert_eq!(e(&mut db, &["d", "2", "z"]), "ERR syntax error"); + for opts in [ + &["WEIGHTS", "1"][..], + &["AGGREGATE", "SUM"], + &["WITHSCORES"], + &["BOGUS"], + ] { + let mut a = vec!["d", "1", "z"]; + a.extend_from_slice(opts); + assert_eq!(e(&mut db, &a), "ERR syntax error", "{opts:?}"); + } + assert!( + !exists(&mut db, "d"), + "no error may have created the destination" + ); + // WRONGTYPE from either position, and it outranks an option error: + // Redis looks the sources up before it parses the options. + assert!(e(&mut db, &["d", "2", "str", "z"]).starts_with("WRONGTYPE")); + assert!(e(&mut db, &["d", "2", "z", "str"]).starts_with("WRONGTYPE")); + assert!(e(&mut db, &["d", "1", "str", "BOGUS"]).starts_with("WRONGTYPE")); + // ... while a numkeys error or an overrun is decided before the lookup. + assert_eq!( + e(&mut db, &["d", "0", "str", "BOGUS"]), + "ERR at least 1 input key is needed for 'zdiffstore' command" + ); + assert_eq!(e(&mut db, &["d", "2", "str"]), "ERR syntax error"); + // The destination's type is irrelevant until the write. + assert_eq!(e(&mut db, &["str", "1", "z", "BOGUS"]), "ERR syntax error"); + assert!(!exists(&mut db, "d")); + } + + /// The precedence fix above applies to the whole family, since the three + /// share one implementation: `ZUNIONSTORE d 1 BOGUS` is + /// WRONGTYPE on redis 8.6.1, and was `syntax error` on moon. + #[test] + fn zunionstore_wrongtype_outranks_an_option_error() { + let mut db = Database::new(); + seed(&mut db, "z", FIVE); + call(&mut db, "SET", &["str", "v"]); + for (cmd, opts) in [ + ("ZUNIONSTORE", &["BOGUS"][..]), + ("ZUNIONSTORE", &["WEIGHTS", "nan"]), + ("ZINTERSTORE", &["WEIGHTS", "1"]), + ] { + let mut a = vec!["d", "2", "z", "str"]; + a.extend_from_slice(opts); + assert!( + err_text(&call(&mut db, cmd, &a)).starts_with("WRONGTYPE"), + "{cmd} {opts:?}" + ); + } + // A well-typed source with a bad option is still the option's error. + assert_eq!( + err_text(&call(&mut db, "ZUNIONSTORE", &["d", "1", "z", "BOGUS"])), + "ERR syntax error" + ); + assert_eq!( + err_text(&call( + &mut db, + "ZUNIONSTORE", + &["d", "1", "z", "WEIGHTS", "nan"] + )), + "ERR weight value is not a float" + ); + // And reading a listpack source through the store family leaves it a + // listpack (the moon#928 defect, closed for this family too). + assert_eq!( + call(&mut db, "ZUNIONSTORE", &["u", "1", "z"]), + Frame::Integer(5) + ); + assert_eq!(encoding_of(&mut db, "z"), "listpack"); + } + + // ── ZADD ... INCR ──────────────────────────────────────────────────── + + fn bulk_text(frame: &Frame) -> String { + match frame { + Frame::BulkString(b) => String::from_utf8_lossy(b).into_owned(), + other => panic!("expected a bulk string, got {other:?}"), + } + } + + #[test] + fn zadd_incr_replies_the_new_score_on_both_encodings() { + for promote in [false, true] { + let mut db = Database::new(); + let key = "i"; + if promote { + call(&mut db, "ZADD", &[key, "1", LONG]); + assert_eq!(encoding_of(&mut db, key), "skiplist"); + } + let incr = |db: &mut Database, a: &[&str]| call(db, "ZADD", a); + assert_eq!(bulk_text(&incr(&mut db, &[key, "INCR", "5", "a"])), "5"); + assert_eq!(bulk_text(&incr(&mut db, &[key, "INCR", "2.5", "a"])), "7.5"); + assert_eq!( + bulk_text(&incr(&mut db, &[key, "INCR", "1e3", "big"])), + "1000" + ); + assert_eq!( + bulk_text(&incr(&mut db, &[key, "INCR", "0.1", "big"])), + "1000.1" + ); + // Option order does not matter, and CH has no say in the reply. + assert_eq!( + bulk_text(&incr(&mut db, &[key, "CH", "INCR", "1", "a"])), + "8.5" + ); + assert_eq!( + bulk_text(&incr(&mut db, &[key, "INCR", "CH", "1", "a"])), + "9.5" + ); + assert_eq!(bulk_text(&incr(&mut db, &[key, "incr", "1", "a"])), "10.5"); + assert_eq!(bulk_text(&call(&mut db, "ZSCORE", &[key, "a"])), "10.5"); + if !promote { + assert_eq!(encoding_of(&mut db, key), "listpack"); + } + ledger_exact(&mut db, &format!("after ZADD INCR promote={promote}")); + } + } + + #[test] + fn zadd_incr_honours_nx_xx_gt_lt_like_redis() { + for promote in [false, true] { + let mut db = Database::new(); + let key = "i"; + if promote { + call(&mut db, "ZADD", &[key, "1", LONG]); + } + let incr = |db: &mut Database, a: &[&str]| call(db, "ZADD", a); + assert_eq!(bulk_text(&incr(&mut db, &[key, "INCR", "5", "a"])), "5"); + // NX: refuses a present member, admits a new one. + assert_eq!(incr(&mut db, &[key, "NX", "INCR", "1", "a"]), Frame::Null); + assert_eq!( + bulk_text(&incr(&mut db, &[key, "NX", "INCR", "1", "newm"])), + "1" + ); + // XX: refuses a new member, admits a present one. + assert_eq!( + incr(&mut db, &[key, "XX", "INCR", "1", "nope"]), + Frame::Null + ); + assert_eq!( + bulk_text(&incr(&mut db, &[key, "XX", "INCR", "1", "a"])), + "6" + ); + // GT/LT: only a move in the right direction; zero is a refusal. + assert_eq!(incr(&mut db, &[key, "GT", "INCR", "-1", "a"]), Frame::Null); + assert_eq!( + bulk_text(&incr(&mut db, &[key, "GT", "INCR", "1", "a"])), + "7" + ); + assert_eq!(incr(&mut db, &[key, "LT", "INCR", "1", "a"]), Frame::Null); + assert_eq!( + bulk_text(&incr(&mut db, &[key, "LT", "INCR", "-1", "a"])), + "6" + ); + assert_eq!(incr(&mut db, &[key, "GT", "INCR", "0", "a"]), Frame::Null); + assert_eq!(incr(&mut db, &[key, "LT", "INCR", "0", "a"]), Frame::Null); + // GT/LT never block a first insert; XX+GT does. + assert_eq!( + bulk_text(&incr(&mut db, &[key, "GT", "INCR", "1", "zz"])), + "1" + ); + assert_eq!( + bulk_text(&incr(&mut db, &[key, "LT", "INCR", "1", "yy"])), + "1" + ); + assert_eq!( + incr(&mut db, &[key, "XX", "GT", "INCR", "1", "qq"]), + Frame::Null + ); + // A refusal wrote nothing. + assert_eq!(bulk_text(&call(&mut db, "ZSCORE", &[key, "a"])), "6"); + assert_eq!(call(&mut db, "ZSCORE", &[key, "qq"]), Frame::Null); + ledger_exact( + &mut db, + &format!("after flagged ZADD INCR promote={promote}"), + ); + } + } + + #[test] + fn zadd_incr_refusal_on_a_missing_key_creates_nothing() { + let mut db = Database::new(); + assert_eq!( + call(&mut db, "ZADD", &["i3", "XX", "INCR", "1", "a"]), + Frame::Null + ); + assert!(!exists(&mut db, "i3")); + // The B+tree arm too: a member too long for a listpack. + assert_eq!( + call(&mut db, "ZADD", &["i4", "XX", "INCR", "1", LONG]), + Frame::Null + ); + assert!(!exists(&mut db, "i4")); + assert_eq!( + bulk_text(&call(&mut db, "ZADD", &["i3", "NX", "INCR", "1", "a"])), + "1" + ); + ledger_exact(&mut db, "after refused ZADD INCR on missing keys"); + } + + #[test] + fn zadd_incr_error_surface_matches_the_oracle() { + let mut db = Database::new(); + seed(&mut db, "i", &[("1", "a")]); + call(&mut db, "SET", &["str", "v"]); + let e = |db: &mut Database, a: &[&str]| err_text(&call(db, "ZADD", a)); + assert_eq!( + e(&mut db, &["i", "INCR", "1", "a", "2", "b"]), + "ERR INCR option supports a single increment-element pair" + ); + assert_eq!( + e(&mut db, &["i", "INCR"]), + "ERR wrong number of arguments for 'zadd' command" + ); + // Parity is checked before the pair count ... + assert_eq!(e(&mut db, &["i", "INCR", "1"]), "ERR syntax error"); + assert_eq!( + e(&mut db, &["i", "INCR", "1", "a", "2"]), + "ERR syntax error" + ); + // ... and the flag pairings before both. + assert_eq!( + e(&mut db, &["i", "INCR", "NX", "XX", "1", "a", "2", "b"]), + "ERR XX and NX options at the same time are not compatible" + ); + assert_eq!( + e(&mut db, &["i", "INCR", "GT", "LT", "1", "a"]), + "ERR GT, LT, and/or NX options at the same time are not compatible" + ); + assert_eq!( + e(&mut db, &["i", "INCR", "nan", "a"]), + "ERR value is not a valid float" + ); + assert_eq!( + e(&mut db, &["i", "INCR", "notafloat", "a"]), + "ERR value is not a valid float" + ); + assert!(e(&mut db, &["str", "INCR", "1", "a"]).starts_with("WRONGTYPE")); + // inf + -inf is NaN: refused with the ZINCRBY message, score untouched. + assert_eq!( + bulk_text(&call(&mut db, "ZADD", &["i", "INCR", "inf", "a"])), + "inf" + ); + assert_eq!( + e(&mut db, &["i", "INCR", "-inf", "a"]), + "ERR resulting score is not a number (NaN)" + ); + assert_eq!(bulk_text(&call(&mut db, "ZSCORE", &["i", "a"])), "inf"); + assert_eq!( + encoding_of(&mut db, "i"), + "listpack", + "an erroring INCR must not flatten" + ); + // NX outranks the NaN check: the sum is never formed for a present + // member under NX. + assert_eq!( + call(&mut db, "ZADD", &["i", "NX", "INCR", "-inf", "a"]), + Frame::Null + ); + } + + /// The plain ZINCRBY went through the refactored core; its contract is + /// unchanged. + #[test] + fn zincrby_is_unchanged_by_the_shared_core() { + let mut db = Database::new(); + assert_eq!(bulk_text(&call(&mut db, "ZINCRBY", &["z", "5", "a"])), "5"); + assert_eq!( + bulk_text(&call(&mut db, "ZINCRBY", &["z", "-2.5", "a"])), + "2.5" + ); + assert_eq!( + bulk_text(&call(&mut db, "ZINCRBY", &["z", "inf", "a"])), + "inf" + ); + assert_eq!( + err_text(&call(&mut db, "ZINCRBY", &["z", "-inf", "a"])), + "ERR resulting score is not a number (NaN)" + ); + assert_eq!( + err_text(&call(&mut db, "ZINCRBY", &["z", "nan", "a"])), + "ERR value is not a valid float" + ); + assert_eq!(encoding_of(&mut db, "z"), "listpack"); + assert_eq!(bulk_text(&call(&mut db, "ZINCRBY", &["z", "1", LONG])), "1"); + assert_eq!(encoding_of(&mut db, "z"), "skiplist"); + assert_eq!( + bulk_text(&call(&mut db, "ZINCRBY", &["z", "1", "a"])), + "inf" + ); + ledger_exact(&mut db, "after ZINCRBY through the shared core"); + } + + #[test] + fn rank_window_follows_the_redis_rule() { + assert_eq!(rank_window(0, 0, 5), Some((0, 0))); + assert_eq!(rank_window(-2, -1, 5), Some((3, 4))); + assert_eq!(rank_window(3, 1, 5), None); + assert_eq!(rank_window(0, 100, 5), Some((0, 4))); + assert_eq!(rank_window(-100, 1, 5), Some((0, 1))); + assert_eq!(rank_window(5, 10, 5), None); + assert_eq!(rank_window(-1, -3, 5), None); + assert_eq!(rank_window(-10, -6, 5), None); + assert_eq!(rank_window(2, -2, 5), Some((2, 3))); + assert_eq!(rank_window(0, -1, 0), None); + assert_eq!(rank_window(i64::MIN, i64::MAX, 5), Some((0, 4))); + assert_eq!(rank_window(i64::MAX, i64::MAX, 5), None); + } +} diff --git a/src/command/sorted_set/sorted_set_lex.rs b/src/command/sorted_set/sorted_set_lex.rs new file mode 100644 index 000000000..f2a745b5c --- /dev/null +++ b/src/command/sorted_set/sorted_set_lex.rs @@ -0,0 +1,157 @@ +//! `ZRANGEBYLEX` and `ZREVRANGEBYLEX` (moon#959). +//! +//! Own file rather than `sorted_set_read.rs`, which already sits at the +//! 1500-line rule. Same shape as `zrangebyscore_readonly`: the mutable-path +//! entry delegates to the shared-borrow twin so a listpack zset survives the +//! read (moon#928), and both encodings answer through the ONE pair of range +//! helpers `ZRANGE ... BYLEX` already uses, so the legacy spelling and the +//! unified one cannot drift apart. Tests stay in `mod.rs`. + +use crate::framevec; +use crate::protocol::Frame; +use crate::storage::Database; + +use crate::command::helpers::{err, err_wrong_args, extract_bytes}; + +use super::{parse_lex_bound, zrange_by_lex, zrange_from_entries}; + +/// ZRANGEBYLEX key min max [LIMIT offset count]. +/// +/// Reads through the shared-borrow implementation so the zset's compact +/// encoding survives the read (moon#928). +pub fn zrangebylex(db: &mut Database, args: &[Frame]) -> Frame { + let now_ms = db.now_ms(); + zrangebylex_readonly(db, args, now_ms) +} + +/// ZREVRANGEBYLEX key max min [LIMIT offset count]. +/// +/// Reads through the shared-borrow implementation so the zset's compact +/// encoding survives the read (moon#928). +pub fn zrevrangebylex(db: &mut Database, args: &[Frame]) -> Frame { + let now_ms = db.now_ms(); + zrevrangebylex_readonly(db, args, now_ms) +} + +/// ZRANGEBYLEX (read-only). +pub fn zrangebylex_readonly(db: &Database, args: &[Frame], now_ms: u64) -> Frame { + zrangebylex_impl(db, args, now_ms, false) +} + +/// ZREVRANGEBYLEX (read-only). +pub fn zrevrangebylex_readonly(db: &Database, args: &[Frame], now_ms: u64) -> Frame { + zrangebylex_impl(db, args, now_ms, true) +} + +/// The one implementation behind both spellings. +/// +/// Error precedence follows Redis's `zrangeGenericCommand`, verified against +/// redis-server 8.6.1: the option loop first (a dangling `LIMIT` or an unknown +/// token is `syntax error`, a non-integer `LIMIT` value is the generic integer +/// error), then the range grammar (`min or max not valid string range item`), +/// then `WITHSCORES` — which the legacy spelling parses but refuses with its +/// own message — and only then the key. The bounds are therefore validated +/// BEFORE the lookup, so `ZRANGEBYLEX nokey a b` is an error and not an empty +/// array. +fn zrangebylex_impl(db: &Database, args: &[Frame], now_ms: u64, rev: bool) -> Frame { + let cmd = if rev { "ZREVRANGEBYLEX" } else { "ZRANGEBYLEX" }; + if args.len() < 3 { + return err_wrong_args(cmd); + } + let key = match extract_bytes(&args[0]) { + Some(k) => k, + None => return err_wrong_args(cmd), + }; + // ZREVRANGEBYLEX takes `max min`; the range helpers take `(min, max)` in + // semantic order and only ever use `rev` for iteration direction, exactly + // as `zrevrangebyscore_readonly` does. + let (min_idx, max_idx) = if rev { (2, 1) } else { (1, 2) }; + let min_arg = match extract_bytes(&args[min_idx]) { + Some(b) => b, + None => return err_wrong_args(cmd), + }; + let max_arg = match extract_bytes(&args[max_idx]) { + Some(b) => b, + None => return err_wrong_args(cmd), + }; + + let mut withscores = false; + let mut limit_offset: Option = None; + let mut limit_count: Option = None; + let mut i = 3; + while i < args.len() { + let opt = match extract_bytes(&args[i]) { + Some(b) => b.as_ref(), + None => return err("ERR syntax error"), + }; + if opt.eq_ignore_ascii_case(b"LIMIT") { + // A `LIMIT` with fewer than two values is `syntax error`, not an + // arity error: the arity floor was already met above. + if i + 2 >= args.len() { + return err("ERR syntax error"); + } + let (Some(off_b), Some(cnt_b)) = + (extract_bytes(&args[i + 1]), extract_bytes(&args[i + 2])) + else { + return err("ERR syntax error"); + }; + limit_offset = std::str::from_utf8(off_b).ok().and_then(|s| s.parse().ok()); + limit_count = std::str::from_utf8(cnt_b).ok().and_then(|s| s.parse().ok()); + if limit_offset.is_none() || limit_count.is_none() { + return err("ERR value is not an integer or out of range"); + } + i += 3; + } else if opt.eq_ignore_ascii_case(b"WITHSCORES") { + // Parsed here, refused below: the range grammar is checked first. + withscores = true; + i += 1; + } else { + return err("ERR syntax error"); + } + } + + // Validate the grammar before the key is consulted. The helpers below + // parse the bounds again; that second pass is two small copies on a path + // that is about to materialise the reply, and it keeps the ONE grammar + // `ZRANGE ... BYLEX` uses rather than a second parser to drift from it. + if let Err(e) = parse_lex_bound(min_arg) { + return e; + } + if let Err(e) = parse_lex_bound(max_arg) { + return e; + } + if withscores { + return err("ERR syntax error, WITHSCORES not supported in combination with BYLEX"); + } + + match db.get_sorted_set_ref_if_alive(key, now_ms) { + Ok(Some(zref)) => match (zref.members_map(), zref.bptree()) { + (Some(members), Some(scores)) => zrange_by_lex( + scores, + min_arg, + max_arg, + rev, + false, + members, + limit_offset, + limit_count, + ), + _ => { + let entries = zref.entries_sorted(); + zrange_from_entries( + &entries, + min_arg, + max_arg, + false, + true, + rev, + false, + limit_offset, + limit_count, + ) + } + }, + Ok(None) => Frame::Array(framevec![]), + Err(e) => e, + } +} diff --git a/src/command/sorted_set/sorted_set_store.rs b/src/command/sorted_set/sorted_set_store.rs new file mode 100644 index 000000000..6d4105b22 --- /dev/null +++ b/src/command/sorted_set/sorted_set_store.rs @@ -0,0 +1,465 @@ +//! The sorted-set STORE family: `ZUNIONSTORE`, `ZINTERSTORE`, `ZDIFFSTORE` +//! and `ZRANGESTORE`. +//! +//! Split out of `sorted_set_write.rs` when `ZDIFFSTORE` and the +//! `ZREMRANGEBY*` trio (moon#959) took that file past the 1500-line rule. +//! Every command here reads one or more SOURCE zsets and REPLACES a +//! destination, which is a different shape from the in-place writes that +//! stay in the write half. Tests stay in `mod.rs`. + +use bytes::Bytes; +use std::collections::HashMap; + +use crate::protocol::Frame; +use crate::storage::Database; +use crate::storage::db::{zset_member_cost, zset_table_bytes}; + +use crate::command::helpers::{err, err_wrong_args, extract_bytes}; + +use super::{ + AggregateOp, clamp_nan_to_zero, parse_numkeys, zadd_member, zrange_by_lex, zrange_by_rank, + zrange_by_score, +}; + +/// Which set operation a `Z*STORE` command computes over its sources. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum SetOp { + Union, + Inter, + /// Members of the FIRST source that are absent from every other one, + /// keeping the first source's scores. Takes no `WEIGHTS`/`AGGREGATE`: + /// Redis's `zunionInterDiffGenericCommand` recognises those tokens only + /// when `op != SET_OP_DIFF`, so on `ZDIFFSTORE` they are `ERR syntax + /// error` like any other unknown token (verified against redis-server + /// 8.6.1, moon#959). + Diff, +} + +impl SetOp { + /// The registered command name, for the arity and `numkeys` messages. + fn name(self) -> &'static str { + match self { + SetOp::Union => "ZUNIONSTORE", + SetOp::Inter => "ZINTERSTORE", + SetOp::Diff => "ZDIFFSTORE", + } + } +} + +/// ZUNIONSTORE destination numkeys key [key ...] [WEIGHTS weight ...] [AGGREGATE SUM|MIN|MAX] +pub fn zunionstore(db: &mut Database, args: &[Frame]) -> Frame { + zstore_impl(db, args, SetOp::Union) +} + +/// ZINTERSTORE destination numkeys key [key ...] [WEIGHTS weight ...] [AGGREGATE SUM|MIN|MAX] +pub fn zinterstore(db: &mut Database, args: &[Frame]) -> Frame { + zstore_impl(db, args, SetOp::Inter) +} + +/// ZDIFFSTORE destination numkeys key [key ...] (moon#959) +/// +/// Stores in `destination` the members of the first source absent from every +/// other source, with the first source's scores. Replies the cardinality of +/// `destination`, deleting it when the difference is empty. Shares the +/// `numkeys` contract and the option loop of its siblings — including the +/// two-class `numkeys` split (moon#969) — with `WEIGHTS`/`AGGREGATE` refused +/// as `syntax error`, which is what redis 8.6.1 answers. +pub fn zdiffstore(db: &mut Database, args: &[Frame]) -> Frame { + zstore_impl(db, args, SetOp::Diff) +} + +fn zstore_impl(db: &mut Database, args: &[Frame], op: SetOp) -> Frame { + let cmd_name = op.name(); + if args.len() < 3 { + return err_wrong_args(cmd_name); + } + let dest = match extract_bytes(&args[0]) { + Some(k) => k, + None => return err_wrong_args(cmd_name), + }; + let numkeys_bytes = match extract_bytes(&args[1]) { + Some(b) => b, + None => return err_wrong_args(cmd_name), + }; + let numkeys = match parse_numkeys(numkeys_bytes, cmd_name) { + Ok(n) => n, + Err(e) => return e, + }; + + // A `numkeys` that overruns the key list is `syntax error`, not an arity + // error (moon#969) — Redis's arity check already passed above, and + // `zunionInterDiffGenericCommand` answers `shared.syntaxerr` here. + if args.len() < 2 + numkeys { + return err("ERR syntax error"); + } + + // Collect source keys + let source_keys: Vec = (0..numkeys) + .map(|j| { + extract_bytes(&args[2 + j]) + .cloned() + .unwrap_or_else(|| Bytes::new()) + }) + .collect(); + + // The sources are read BEFORE the options are parsed, because that is + // the order `zunionInterDiffGenericCommand` takes: it looks every source + // up (and refuses a wrong type) before it looks at `WEIGHTS`, so + // `ZUNIONSTORE d 1 BOGUS` is `WRONGTYPE` on redis 8.6.1, not + // `syntax error`. Parsing the options first inverted that (moon#959). + // + // Each source is read through the SHARED-borrow view, not `get_sorted_set`: that + // accessor's `get_promoted` core upgrades a listpack source to the B+tree + // form as a side effect of READING it — the moon#928 defect, which the + // read-only set-operation family (`collect_source_sets_readonly`) already + // left behind. A `&Database` borrow cannot reach `SortedSetKind::upgrade` + // at all, and `get_sorted_set_ref_if_alive` classifies every encoding + // (B+tree, listpack, legacy, and a cold-tier hit read through as `Owned`) + // rather than demanding one. Each source is copied into an owned map + // exactly as before, because the destination write below needs `db` + // mutably; a listpack's copy is bounded by `zset-max-listpack-entries`. + let now_ms = db.now_ms(); + let mut source_data: Vec> = Vec::with_capacity(numkeys); + for key in &source_keys { + match db.get_sorted_set_ref_if_alive(key, now_ms) { + Ok(Some(zref)) => match zref.members_map() { + Some(members) => source_data.push(members.clone()), + None => source_data.push(zref.entries_sorted().into_iter().collect()), + }, + Ok(None) => { + source_data.push(HashMap::new()); + } + Err(e) => return e, + } + } + + // Parse WEIGHTS and AGGREGATE + let mut weights: Vec = vec![1.0; numkeys]; + let mut aggregate = AggregateOp::Sum; + let mut i = 2 + numkeys; + + while i < args.len() { + let opt = match extract_bytes(&args[i]) { + Some(b) => b.as_ref(), + None => { + i += 1; + continue; + } + }; + // `WEIGHTS` and `AGGREGATE` are not tokens ZDIFFSTORE knows — Redis + // only matches them `if (op != SET_OP_DIFF)`, so on a diff they fall + // through to the unknown-token arm and are `syntax error` (moon#959). + let takes_weights = op != SetOp::Diff; + if takes_weights && opt.eq_ignore_ascii_case(b"WEIGHTS") { + for w in 0..numkeys { + // Too few weights to cover the key list is `syntax error` on + // Redis, not an arity error (moon#969). + if i + 1 + w >= args.len() { + return err("ERR syntax error"); + } + let wb = match extract_bytes(&args[i + 1 + w]) { + Some(b) => b, + None => return err("ERR syntax error"), + }; + // `"nan"` PARSES in Rust where C's `strtod` + `isnan` check in + // `getDoubleFromObjectOrReply` rejects it (moon#969), so a NaN + // weight sailed through and poisoned every aggregated score. + // Infinities stay legal, as they are on Redis. + let wval: f64 = match std::str::from_utf8(wb) + .ok() + .and_then(|s| s.parse::().ok()) + .filter(|v| !v.is_nan()) + { + Some(v) => v, + None => return err("ERR weight value is not a float"), + }; + weights[w] = wval; + } + i += 1 + numkeys; + } else if takes_weights && opt.eq_ignore_ascii_case(b"AGGREGATE") { + if i + 1 >= args.len() { + return err("ERR syntax error"); + } + let agg_b = match extract_bytes(&args[i + 1]) { + Some(b) => b.as_ref(), + None => return err("ERR syntax error"), + }; + aggregate = if agg_b.eq_ignore_ascii_case(b"SUM") { + AggregateOp::Sum + } else if agg_b.eq_ignore_ascii_case(b"MIN") { + AggregateOp::Min + } else if agg_b.eq_ignore_ascii_case(b"MAX") { + AggregateOp::Max + } else { + return err("ERR syntax error"); + }; + i += 2; + } else { + // moon#967 rewrote every OTHER zset option loop to reject an + // unrecognised token and missed this one, so `ZUNIONSTORE d 1 k + // BOGUS` stepped over `BOGUS` and answered a DIFFERENT, successful + // command. Redis: `ERR syntax error`. + return err("ERR syntax error"); + } + } + + // Compute result + let mut result_map: HashMap = HashMap::new(); + + if op == SetOp::Diff { + // Members of the first source that no later source contains, with the + // first source's scores untouched — the same walk `zdiff_readonly` + // makes. No weight applies: the option loop refused `WEIGHTS`. + if let Some(first) = source_data.first() { + 'outer: for (member, score) in first { + for src in source_data.iter().skip(1) { + if src.contains_key(member) { + continue 'outer; + } + } + result_map.insert(member.clone(), *score); + } + } + } else if op == SetOp::Inter { + // Start with first set's members + if let Some(first) = source_data.first() { + for (member, score) in first { + let weighted = clamp_nan_to_zero(*score * weights[0]); + let mut final_score = weighted; + let mut in_all = true; + + for (idx, src) in source_data.iter().enumerate().skip(1) { + match src.get(member) { + Some(s) => { + let ws = clamp_nan_to_zero(*s * weights[idx]); + final_score = match aggregate { + AggregateOp::Sum => clamp_nan_to_zero(final_score + ws), + AggregateOp::Min => final_score.min(ws), + AggregateOp::Max => final_score.max(ws), + }; + } + None => { + in_all = false; + break; + } + } + } + + if in_all { + result_map.insert(member.clone(), final_score); + } + } + } + } else { + // Union: all members from all sets + for (idx, src) in source_data.iter().enumerate() { + for (member, score) in src { + let weighted = clamp_nan_to_zero(*score * weights[idx]); + result_map + .entry(member.clone()) + .and_modify(|existing| { + *existing = match aggregate { + AggregateOp::Sum => clamp_nan_to_zero(*existing + weighted), + AggregateOp::Min => existing.min(weighted), + AggregateOp::Max => existing.max(weighted), + }; + }) + .or_insert(weighted); + } + } + } + + let result_size = result_map.len() as i64; + + // Remove destination key first, then create new sorted set + db.remove(dest); + + if !result_map.is_empty() { + let (members, scores) = match db.get_or_create_sorted_set(dest) { + Ok(pair) => pair, + Err(e) => return e, + }; + + // `dest` was just removed/recreated above, so every member here is + // new -- charge each unconditionally (O(1) per member, no full + // recompute of the destination sorted set). + let mut mem_charge: usize = 0; + let table_before = zset_table_bytes(members, scores); + for (member, score) in result_map { + mem_charge += zset_member_cost(&member); + zadd_member(members, scores, member, score); + } + let table_after = zset_table_bytes(members, scores); + // `members`/`scores`' borrow of `db` ends above. + db.charge_memory(mem_charge); + db.adjust_memory(table_before, table_after); + } + + Frame::Integer(result_size) +} + +// --------------------------------------------------------------------------- +// ZRANGESTORE dst src min max [BYSCORE | BYLEX] [REV] [LIMIT offset count] +// --------------------------------------------------------------------------- + +/// ZRANGESTORE dst src min max [BYSCORE | BYLEX] [REV] [LIMIT offset count] +/// +/// Stores the result of a ZRANGE into `dst`, replacing it. Returns the cardinality of `dst`. +pub fn zrangestore(db: &mut Database, args: &[Frame]) -> Frame { + if args.len() < 4 { + return err_wrong_args("ZRANGESTORE"); + } + let dst = match extract_bytes(&args[0]) { + Some(k) => k, + None => return err_wrong_args("ZRANGESTORE"), + }; + let src = match extract_bytes(&args[1]) { + Some(k) => k, + None => return err_wrong_args("ZRANGESTORE"), + }; + let min_arg = match extract_bytes(&args[2]) { + Some(b) => b.clone(), + None => return err_wrong_args("ZRANGESTORE"), + }; + let max_arg = match extract_bytes(&args[3]) { + Some(b) => b.clone(), + None => return err_wrong_args("ZRANGESTORE"), + }; + + // Parse optional flags (same as ZRANGE but no WITHSCORES) + let mut by_score = false; + let mut by_lex = false; + let mut rev = false; + let mut limit_offset: Option = None; + let mut limit_count: Option = None; + + let mut i = 4; + while i < args.len() { + let opt = match extract_bytes(&args[i]) { + Some(b) => b.as_ref(), + None => { + i += 1; + continue; + } + }; + if opt.eq_ignore_ascii_case(b"BYSCORE") { + by_score = true; + i += 1; + } else if opt.eq_ignore_ascii_case(b"BYLEX") { + by_lex = true; + i += 1; + } else if opt.eq_ignore_ascii_case(b"REV") { + rev = true; + i += 1; + } else if opt.eq_ignore_ascii_case(b"LIMIT") { + if i + 2 < args.len() { + let off_b = match extract_bytes(&args[i + 1]) { + Some(b) => b, + None => return err_wrong_args("ZRANGESTORE"), + }; + let cnt_b = match extract_bytes(&args[i + 2]) { + Some(b) => b, + None => return err_wrong_args("ZRANGESTORE"), + }; + limit_offset = std::str::from_utf8(off_b).ok().and_then(|s| s.parse().ok()); + limit_count = std::str::from_utf8(cnt_b).ok().and_then(|s| s.parse().ok()); + if limit_offset.is_none() || limit_count.is_none() { + return err("ERR value is not an integer or out of range"); + } + i += 3; + } else { + return err_wrong_args("ZRANGESTORE"); + } + } else { + return err("ERR syntax error"); + } + } + + if by_score && by_lex { + return err("ERR BYSCORE and BYLEX options are not compatible"); + } + if limit_offset.is_some() && !by_score && !by_lex { + return err( + "ERR syntax error, LIMIT is only supported in combination with either BYSCORE or BYLEX", + ); + } + + // Run ZRANGE on src, collecting (member, score) pairs + let entries: Vec<(Bytes, f64)> = match db.get_sorted_set(src) { + Ok(Some((members, scores))) => { + let frame = if by_score { + zrange_by_score( + members, + scores, + &min_arg, + &max_arg, + rev, + true, + limit_offset, + limit_count, + ) + } else if by_lex { + zrange_by_lex( + scores, + &min_arg, + &max_arg, + rev, + true, + members, + limit_offset, + limit_count, + ) + } else { + zrange_by_rank(scores, &min_arg, &max_arg, rev, true) + }; + // Parse the Frame::Array([member, score, member, score, ...]) into Vec<(Bytes, f64)> + match frame { + Frame::Array(arr) => { + let mut result = Vec::with_capacity(arr.len() / 2); + let mut idx = 0; + while idx + 1 < arr.len() { + if let (Frame::BulkString(m), Frame::BulkString(s)) = + (&arr[idx], &arr[idx + 1]) + { + if let Ok(score) = std::str::from_utf8(s).unwrap_or("0").parse::() + { + result.push((m.clone(), score)); + } + } + idx += 2; + } + result + } + Frame::Error(_) => return frame, + _ => Vec::with_capacity(0), + } + } + Ok(None) => Vec::with_capacity(0), + Err(e) => return e, + }; + + let count = entries.len() as i64; + + // Replace dst with the result + db.remove(dst); + + if !entries.is_empty() { + let (dst_members, dst_scores) = match db.get_or_create_sorted_set(dst) { + Ok(pair) => pair, + Err(e) => return e, + }; + // `dst` was just removed/recreated above, so every entry is new. + let mut mem_charge: usize = 0; + let table_before = zset_table_bytes(dst_members, dst_scores); + for (member, score) in entries { + mem_charge += zset_member_cost(&member); + zadd_member(dst_members, dst_scores, member, score); + } + let table_after = zset_table_bytes(dst_members, dst_scores); + // `dst_members`/`dst_scores`' borrow of `db` ends above. + db.charge_memory(mem_charge); + db.adjust_memory(table_before, table_after); + } + + Frame::Integer(count) +} diff --git a/src/command/sorted_set/sorted_set_write.rs b/src/command/sorted_set/sorted_set_write.rs index c0cd5add3..e41bf4ee1 100644 --- a/src/command/sorted_set/sorted_set_write.rs +++ b/src/command/sorted_set/sorted_set_write.rs @@ -1,9 +1,9 @@ use bytes::Bytes; -use std::collections::HashMap; +use ordered_float::OrderedFloat; use crate::protocol::Frame; use crate::storage::Database; -use crate::storage::db::{Shape, zset_member_cost, zset_table_bytes}; +use crate::storage::db::{Shape, SortedSetRef, zset_member_cost, zset_table_bytes}; use crate::storage::listpack::PairUpdate; use crate::storage::zset_score::{ScoreBuf, render_score}; @@ -11,9 +11,9 @@ use crate::command::helpers::{all_args_are_bytes, err, err_wrong_args, extract_b use crate::command::sorted_set::work_budget; use super::{ - AggregateOp, clamp_nan_to_zero, format_score, format_score_bytes, parse_bounded_count, - parse_numkeys, zadd_member, zrange_by_lex, zrange_by_rank, zrange_by_score, zrem_member, - zset_insert_absent, zset_update_existing, + LexBound, ScoreBound, format_score, format_score_bytes, lex_in_range, parse_bounded_count, + parse_lex_bound, parse_score_bound, rank_window, zrem_member, zset_insert_absent, + zset_update_existing, }; // --------------------------------------------------------------------------- @@ -103,7 +103,7 @@ fn resolved_pair<'a>( // through `render_score`); the closures below read it as 0.0 so the member is // still FOUND and updated in place rather than duplicated. -/// ZADD key [NX|XX] [GT|LT] [CH] score member [score member ...] +/// ZADD key [NX|XX] [GT|LT] [CH] [INCR] score member [score member ...] pub fn zadd(db: &mut Database, args: &[Frame]) -> Frame { if args.len() < 3 { return err_wrong_args("ZADD"); @@ -119,6 +119,7 @@ pub fn zadd(db: &mut Database, args: &[Frame]) -> Frame { let mut gt = false; let mut lt = false; let mut ch = false; + let mut incr = false; let mut i = 1; while i < args.len() { @@ -141,6 +142,9 @@ pub fn zadd(db: &mut Database, args: &[Frame]) -> Frame { } else if arg.eq_ignore_ascii_case(b"CH") { ch = true; i += 1; + } else if arg.eq_ignore_ascii_case(b"INCR") { + incr = true; + i += 1; } else { break; } @@ -170,6 +174,26 @@ pub fn zadd(db: &mut Database, args: &[Frame]) -> Frame { return err("ERR syntax error"); } + // `INCR` (moon#959): ZINCRBY's arithmetic under ZADD's flags, replying the + // new score as a bulk string, or nil when a flag refused the write. Redis + // checks the pair count AFTER the parity and flag-pairing rules above, so + // `ZADD k INCR 1` is `syntax error` and `ZADD k INCR NX XX 1 a 2 b` is the + // NX/XX error — both verified on redis 8.6.1. The increment is parsed by + // the ONE parser every `score member` pair goes through, so a NaN or a + // non-float is refused with `ZADD`'s own message before the keyspace is + // touched. + if incr { + if remaining.len() != 2 { + return err("ERR INCR option supports a single increment-element pair"); + } + let (increment, member) = match parse_zadd_pair(&remaining[0], &remaining[1]) { + Ok(pair) => pair, + Err(e) => return e, + }; + // `CH` has no effect on the INCR reply, as on Redis. + return zincr_member(db, key, increment, member, IncrFlags { nx, xx, gt, lt }); + } + // moon#814: validate EVERY pair BEFORE touching the keyspace. // // The mutation loop below runs inside the `table_before … charge_memory()` @@ -628,6 +652,229 @@ pub fn zrem(db: &mut Database, args: &[Frame]) -> Frame { Frame::Integer(removed) } +// --------------------------------------------------------------------------- +// ZREMRANGEBYRANK / ZREMRANGEBYSCORE / ZREMRANGEBYLEX (moon#959) +// --------------------------------------------------------------------------- + +/// The window a `ZREMRANGEBY*` command deletes, parsed BEFORE the keyspace is +/// touched so a bad bound never fabricates or reclaims a key — Redis parses +/// the range first and only then looks the key up, so `ZREMRANGEBYSCORE +/// nokey a 1` is `min or max is not a float` and not `0`. +enum RemRange { + Rank(i64, i64), + Score(ScoreBound, ScoreBound), + Lex(LexBound, LexBound), +} + +impl RemRange { + /// The members of a score-sorted decode that fall inside the window. + /// Borrowed from `entries`, which is the caller's own copy, so the + /// listpack they came from can be mutated while these are consumed. + fn select<'a>(&self, entries: &'a [(Bytes, f64)]) -> Vec<&'a Bytes> { + match self { + RemRange::Rank(start, stop) => match rank_window(*start, *stop, entries.len()) { + Some((lo, hi)) => entries[lo..=hi].iter().map(|(m, _)| m).collect(), + None => Vec::new(), + }, + RemRange::Score(min, max) => entries + .iter() + .filter(|(_, s)| min.includes(*s) && max.includes_upper(*s)) + .map(|(m, _)| m) + .collect(), + RemRange::Lex(min, max) => entries + .iter() + .filter(|(m, _)| lex_in_range(m, min, max)) + .map(|(m, _)| m) + .collect(), + } + } +} + +/// ZREMRANGEBYRANK key start stop +pub fn zremrangebyrank(db: &mut Database, args: &[Frame]) -> Frame { + let (key, min_b, max_b) = match zremrange_args(args, "ZREMRANGEBYRANK") { + Ok(v) => v, + Err(e) => return e, + }; + // A rank index is read with a NULL message on Redis, so the generic + // integer error is the right class here (moon#969 documents the same for + // ZRANGE's indices). + let parse = |b: &[u8]| -> Result { + std::str::from_utf8(b) + .ok() + .and_then(|s| s.parse().ok()) + .ok_or_else(|| err("ERR value is not an integer or out of range")) + }; + let (start, stop) = match (parse(min_b), parse(max_b)) { + (Ok(a), Ok(b)) => (a, b), + (Err(e), _) | (_, Err(e)) => return e, + }; + zremrange_impl(db, key, RemRange::Rank(start, stop)) +} + +/// ZREMRANGEBYSCORE key min max +pub fn zremrangebyscore(db: &mut Database, args: &[Frame]) -> Frame { + let (key, min_b, max_b) = match zremrange_args(args, "ZREMRANGEBYSCORE") { + Ok(v) => v, + Err(e) => return e, + }; + let (min, max) = match (parse_score_bound(min_b), parse_score_bound(max_b)) { + (Ok(a), Ok(b)) => (a, b), + (Err(e), _) | (_, Err(e)) => return e, + }; + zremrange_impl(db, key, RemRange::Score(min, max)) +} + +/// ZREMRANGEBYLEX key min max +pub fn zremrangebylex(db: &mut Database, args: &[Frame]) -> Frame { + let (key, min_b, max_b) = match zremrange_args(args, "ZREMRANGEBYLEX") { + Ok(v) => v, + Err(e) => return e, + }; + let (min, max) = match (parse_lex_bound(min_b), parse_lex_bound(max_b)) { + (Ok(a), Ok(b)) => (a, b), + (Err(e), _) | (_, Err(e)) => return e, + }; + zremrange_impl(db, key, RemRange::Lex(min, max)) +} + +/// The `key min max` shape all three share: exactly three arguments (their +/// registered arity is 4), every one a bulk string. +fn zremrange_args<'a>( + args: &'a [Frame], + cmd: &'static str, +) -> Result<(&'a Bytes, &'a Bytes, &'a Bytes), Frame> { + if args.len() != 3 { + return Err(err_wrong_args(cmd)); + } + match ( + extract_bytes(&args[0]), + extract_bytes(&args[1]), + extract_bytes(&args[2]), + ) { + (Some(k), Some(min), Some(max)) => Ok((k, min, max)), + _ => Err(err_wrong_args(cmd)), + } +} + +/// Delete every member inside `range` and reply how many went. +/// +/// The same two-arm shape as `zrem` (moon#897): a listpack is trimmed in +/// place and never converted — a removal cannot cross a threshold upward — +/// and the B+tree arm credits each member's cost and the table shrink exactly +/// as `zrem` does. A key that drains to empty is removed on both arms, which +/// is also what reclaims the empty container `get_or_create_zset_listpack` +/// fabricates for a missing key, so `ZREMRANGEBYRANK nokey 0 1` answers `0` +/// and leaves no key behind. +/// +/// The victims are materialised before the first removal on both arms: a +/// listpack keeps insertion order, so the window is decided on a score-sorted +/// decode (bounded by `zset-max-listpack-entries`), and a B+tree cannot be +/// mutated while its iterator is live. The B+tree list holds `Bytes` handles, +/// which are reference-count bumps rather than copies. +fn zremrange_impl(db: &mut Database, key: &[u8], range: RemRange) -> Frame { + match db.get_or_create_zset_listpack(key) { + Ok(Some(lp)) => { + // Listpack `estimate_memory()` is O(1) (capacity-based). + let before = lp.estimate_memory(); + let entries = SortedSetRef::Listpack(&*lp).entries_sorted(); + let mut removed = 0i64; + for member in range.select(&entries) { + // `remove_pair` matches the FIELD half only — the member, + // never the score — and drains both entries in one scan. + if lp.remove_pair(member) { + removed += 1; + } + } + let after = lp.estimate_memory(); + let is_empty = lp.is_empty(); + // `lp`'s borrow of `db` ends here. + db.adjust_memory(before, after); + if is_empty { + db.remove(key); + } + return Frame::Integer(removed); + } + // Already the full B+tree form (or a cold-promoted value, which never + // decodes compact): fall through. + Ok(None) => {} + Err(e) => return e, // WRONGTYPE + } + + let (members, scores) = match db.get_or_create_sorted_set(key) { + Ok(pair) => pair, + Err(e) => return e, + }; + + let victims: Vec = match &range { + RemRange::Rank(start, stop) => match rank_window(*start, *stop, scores.len()) { + Some((lo, hi)) => scores + .range_by_rank(lo, hi) + .into_iter() + .map(|(_, m)| m.clone()) + .collect(), + None => Vec::new(), + }, + RemRange::Score(min, max) => { + // `BPTree::range` wants `lo <= hi`; a reversed pair is an empty + // window on Redis (`ZREMRANGEBYSCORE k 3 1` removes nothing), and + // the bound filters keep an exclusive or infinite edge exact. + let lo = OrderedFloat(min.value()); + let hi = OrderedFloat(max.value()); + if lo > hi { + Vec::new() + } else { + scores + .range(lo, hi) + .filter(|(s, _)| min.includes(s.0) && max.includes_upper(s.0)) + .map(|(_, m)| m.clone()) + .collect() + } + } + RemRange::Lex(min, max) => scores + .iter() + .filter(|(_, m)| lex_in_range(m, min, max)) + .map(|(_, m)| m.clone()) + .collect(), + }; + + let mut removed = 0i64; + let mut credit: usize = 0; + let table_before = zset_table_bytes(members, scores); + for member in &victims { + if zrem_member(members, scores, member) { + removed += 1; + credit += zset_member_cost(member); + } + } + let is_empty = members.is_empty(); + let table_after = zset_table_bytes(members, scores); + // `members`/`scores`' borrow of `db` ends above. + db.credit_memory(credit); + // Unconditional, as in `zrem`: `db.remove` credits `entry_overhead` + // recomputed from the CURRENT value, and a shrunken table's capacity + // must be credited here or it is stranded. + db.adjust_memory(table_before, table_after); + if is_empty { + db.remove(key); + } + Frame::Integer(removed) +} + +// --------------------------------------------------------------------------- +// ZINCRBY, and the arithmetic core it shares with `ZADD ... INCR` +// --------------------------------------------------------------------------- + +/// The `ZADD` flags that bear on an increment (moon#959). All false for a +/// plain `ZINCRBY`. +#[derive(Debug, Clone, Copy, Default)] +struct IncrFlags { + nx: bool, + xx: bool, + gt: bool, + lt: bool, +} + /// ZINCRBY key increment member pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { if args.len() != 3 { @@ -642,7 +889,7 @@ pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { None => return err_wrong_args("ZINCRBY"), }; let member = match extract_bytes(&args[2]) { - Some(b) => b.clone(), + Some(b) => b, None => return err_wrong_args("ZINCRBY"), }; @@ -658,6 +905,27 @@ pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { return err("ERR value is not a valid float"); } + zincr_member(db, key, increment, member, IncrFlags::default()) +} + +/// Add `increment` to `member`'s score, creating the member at `increment` +/// when it is absent, and reply the new score — or nil when a flag refused +/// the write. +/// +/// The decision order is Redis's `zsetAdd` with `ZADD_IN_INCR`, verified on +/// redis 8.6.1: for a PRESENT member, `NX` refuses before the sum is even +/// formed; then a NaN sum is `ERR resulting score is not a number (NaN)` with +/// nothing written; then `GT`/`LT` refuse a sum that does not move the score +/// the right way (`GT` with a zero increment is a refusal). For an ABSENT +/// member only `XX` refuses; `GT`/`LT` never block a first insert. A refusal +/// on a key this call had to fabricate leaves no key behind. +fn zincr_member( + db: &mut Database, + key: &[u8], + increment: f64, + member: &Bytes, + flags: IncrFlags, +) -> Frame { // Listpack path (moon#897). ZINCRBY is the leaderboard primitive, and // before this it took the eager `get_or_create_sorted_set`: one ZINCRBY // flattened a three-member zset to `skiplist` permanently (nothing @@ -677,6 +945,12 @@ pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { Ok(Some(lp)) => { // Listpack `estimate_memory()` is O(1) (capacity-based). let before = lp.estimate_memory(); + // Why the closure declined, when it did. `Unchanged` alone + // cannot say: it is a NaN sum for a plain ZINCRBY and a flag + // refusal under `ZADD ... INCR`, and the two reply + // differently. + let mut reached_nan = false; + let mut refused = false; // ONE scan (moon#942): the walk that finds the member carries // the byte offsets its score is rewritten at, so there is no // second walk back to an ordinal. @@ -685,23 +959,25 @@ pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { // its RENDERED text, and `render_score(NaN)` writes `NaN`, // which `parse_score` refuses — the score would read back as // 0.0 and the member would silently change value. `increment` - // is already proven non-NaN above, so this is reachable only - // as `±inf + ∓inf`, which in turn means the member already - // exists (a fresh member starts at 0.0) — so declining here - // never leaves a key created-and-abandoned. - // - // Declining hands the case to the B+tree arm below, which is - // byte-for-byte what EVERY ZINCRBY did before this branch - // existed. moon's reply there (`NaN`) diverges from redis - // 8.6.1, which answers - // `ERR resulting score is not a number (NaN)` and leaves the - // score untouched — a real, PRE-EXISTING divergence that this - // change deliberately does not alter, and that a NaN must - // never reach a listpack in the meantime. - let outcome = lp.update_pair_value(&member, |current| { + // is already proven non-NaN by every caller, so this is + // reachable only as `±inf + ∓inf`, which in turn means the + // member already exists (a fresh member starts at 0.0) — so + // declining here never leaves a key created-and-abandoned. + let outcome = lp.update_pair_value(member, |current| { + if flags.nx { + // NX: the member is present, which is all it needs. + refused = true; + return None; + } work_budget::note_stored_score_parse(); - let new_score = current.as_score().unwrap_or(0.0) + increment; + let old = current.as_score().unwrap_or(0.0); + let new_score = old + increment; if new_score.is_nan() { + reached_nan = true; + return None; + } + if (flags.gt && new_score <= old) || (flags.lt && new_score >= old) { + refused = true; return None; } // One stack buffer; `render_score` is byte-identical to @@ -718,17 +994,38 @@ pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { // reply below does not render the score a second time. PairUpdate::Replaced(rendered) => Some(rendered), PairUpdate::Absent => { - // A fresh member starts at 0.0 and `increment` is - // already proven non-NaN, so this rendering can never - // be the NaN the arm above guards against. - let mut rendered = ScoreBuf::new(); - render_score(increment, &mut rendered); - lp.push_back(&member); - lp.push_back(&rendered); - Some(rendered) + if flags.xx { + // XX: never create. The container may be one + // this call fabricated; the empty check below + // reclaims it. + refused = true; + None + } else { + // A fresh member starts at 0.0 and `increment` is + // already proven non-NaN, so this rendering can + // never be the NaN the arm above guards against. + let mut rendered = ScoreBuf::new(); + render_score(increment, &mut rendered); + lp.push_back(member); + lp.push_back(&rendered); + Some(rendered) + } } - // NaN. The listpack was not touched, and this is the - // answer (moon#960): redis 8.6.1 replies + // NaN, or a flag refusal: the listpack was not touched. + PairUpdate::Unchanged => None, + }; + + let after = lp.estimate_memory(); + // The upgrade check, from the same authority as the gate: + // it converts `lp.len()` (member AND score entries) to + // members itself — the moon#896 unit. + let should_upgrade = + stored.is_some() && !limits.listpack_fits(Shape::SortedSet, lp); + let is_empty = lp.is_empty(); + // `lp`'s borrow of `db` ends here. + db.adjust_memory(before, after); + if reached_nan { + // This is the answer (moon#960): redis 8.6.1 replies // `ERR resulting score is not a number (NaN)` and leaves // the score alone. Returning here rather than falling // through also keeps the encoding intact — the B+tree arm @@ -736,36 +1033,38 @@ pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { // so falling through would flatten the listpack // permanently (moon#832: nothing demotes) as a side // effect of a command that errors and stores nothing. - PairUpdate::Unchanged => { - return err("ERR resulting score is not a number (NaN)"); - } - }; - + return err("ERR resulting score is not a number (NaN)"); + } + if is_empty { + // Only reachable as an `XX` refusal on a fabricated + // container — the same rule `zadd` applies. + db.remove(key); + } + if refused { + return Frame::Null; + } + if should_upgrade { + // Self-accounting: the accessor bills the one-time + // listpack -> B+tree swing itself (moon#788/#810). + db.upgrade_zset_listpack_to_bptree(key); + } + // Reply with the bytes we STORED, not a second rendering: + // one copy out of the stack buffer instead of the + // `format_score` -> `String` allocation the B+tree arm + // below still pays (`src/command/` is a no-`String` + // path). `render_score` is pinned byte-identical to + // `format_score_bytes` by + // `listpack_score_rendering_matches_zscore_rendering`, so + // this is the same text either way — and it is now the + // same text a later ZSCORE reads out of the listpack, by + // construction rather than by two formatters agreeing. if let Some(rendered) = stored { - let after = lp.estimate_memory(); - // The upgrade check, from the same authority as the gate: - // it converts `lp.len()` (member AND score entries) to - // members itself — the moon#896 unit. - let should_upgrade = !limits.listpack_fits(Shape::SortedSet, lp); - // `lp`'s borrow of `db` ends here. - db.adjust_memory(before, after); - if should_upgrade { - // Self-accounting: the accessor bills the one-time - // listpack -> B+tree swing itself (moon#788/#810). - db.upgrade_zset_listpack_to_bptree(key); - } - // Reply with the bytes we STORED, not a second rendering: - // one copy out of the stack buffer instead of the - // `format_score` -> `String` allocation the B+tree arm - // below still pays (`src/command/` is a no-`String` - // path). `render_score` is pinned byte-identical to - // `format_score_bytes` by - // `listpack_score_rendering_matches_zscore_rendering`, so - // this is the same text either way — and it is now the - // same text a later ZSCORE reads out of the listpack, by - // construction rather than by two formatters agreeing. return Frame::BulkString(Bytes::copy_from_slice(&rendered)); } + // `stored` is `None` exactly when `reached_nan || refused`, + // both returned above. Kept as a real match rather than an + // `unwrap`, as the mutation loops in `zadd` are. + return Frame::Null; } // Already the full B+tree form (or a cold-promoted value, which // never decodes compact): fall through. @@ -779,7 +1078,7 @@ pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { Err(e) => return e, }; - let member_cost = zset_member_cost(&member); + let member_cost = zset_member_cost(member); let table_before = zset_table_bytes(members, scores); // ONE hash lookup (moon#942): the same lookup that reads the current score // writes `current + increment` back through the slot it found. It used to @@ -791,12 +1090,21 @@ pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { // from the closure is `zset_update_existing`'s "leave it alone", so the // old score survives and neither map is touched. let mut reached_nan = false; - let is_new = zset_update_existing(members, scores, &member, |current| { + let mut refused = false; + let is_new = zset_update_existing(members, scores, member, |current| { + if flags.nx { + refused = true; + return None; + } let candidate = current + increment; if candidate.is_nan() { reached_nan = true; return None; } + if (flags.gt && candidate <= current) || (flags.lt && candidate >= current) { + refused = true; + return None; + } new_score = candidate; Some(candidate) }) @@ -804,19 +1112,28 @@ pub fn zincrby(db: &mut Database, args: &[Frame]) -> Frame { if reached_nan { return err("ERR resulting score is not a number (NaN)"); } - if is_new { + let inserted = is_new && !flags.xx; + if inserted { // A member that was not there starts at 0.0, so its new score is the // increment itself — already in `new_score`. - zset_insert_absent(members, scores, member, new_score); + zset_insert_absent(members, scores, member.clone(), new_score); } + let is_empty = members.is_empty(); let table_after = zset_table_bytes(members, scores); // `members`/`scores`' borrow of `db` ends above. - if is_new { + if inserted { db.charge_memory(member_cost); } db.adjust_memory(table_before, table_after); + if is_empty { + // `XX` refused the only member a fabricated container would have had. + db.remove(key); + } + if refused || (is_new && flags.xx) { + return Frame::Null; + } - Frame::BulkString(Bytes::from(format_score(new_score))) + Frame::BulkString(format_score_bytes(new_score)) } /// ZPOPMIN key [count] @@ -961,381 +1278,6 @@ pub fn zpopmax(db: &mut Database, args: &[Frame]) -> Frame { Frame::Array(result.into()) } -/// ZUNIONSTORE destination numkeys key [key ...] [WEIGHTS weight ...] [AGGREGATE SUM|MIN|MAX] -pub fn zunionstore(db: &mut Database, args: &[Frame]) -> Frame { - zstore_impl(db, args, false) -} - -/// ZINTERSTORE destination numkeys key [key ...] [WEIGHTS weight ...] [AGGREGATE SUM|MIN|MAX] -pub fn zinterstore(db: &mut Database, args: &[Frame]) -> Frame { - zstore_impl(db, args, true) -} - -fn zstore_impl(db: &mut Database, args: &[Frame], intersect: bool) -> Frame { - let cmd_name = if intersect { - "ZINTERSTORE" - } else { - "ZUNIONSTORE" - }; - if args.len() < 3 { - return err_wrong_args(cmd_name); - } - let dest = match extract_bytes(&args[0]) { - Some(k) => k, - None => return err_wrong_args(cmd_name), - }; - let numkeys_bytes = match extract_bytes(&args[1]) { - Some(b) => b, - None => return err_wrong_args(cmd_name), - }; - let numkeys = match parse_numkeys(numkeys_bytes, cmd_name) { - Ok(n) => n, - Err(e) => return e, - }; - - // A `numkeys` that overruns the key list is `syntax error`, not an arity - // error (moon#969) — Redis's arity check already passed above, and - // `zunionInterDiffGenericCommand` answers `shared.syntaxerr` here. - if args.len() < 2 + numkeys { - return err("ERR syntax error"); - } - - // Collect source keys - let source_keys: Vec = (0..numkeys) - .map(|j| { - extract_bytes(&args[2 + j]) - .cloned() - .unwrap_or_else(|| Bytes::new()) - }) - .collect(); - - // Parse WEIGHTS and AGGREGATE - let mut weights: Vec = vec![1.0; numkeys]; - let mut aggregate = AggregateOp::Sum; - let mut i = 2 + numkeys; - - while i < args.len() { - let opt = match extract_bytes(&args[i]) { - Some(b) => b.as_ref(), - None => { - i += 1; - continue; - } - }; - if opt.eq_ignore_ascii_case(b"WEIGHTS") { - for w in 0..numkeys { - // Too few weights to cover the key list is `syntax error` on - // Redis, not an arity error (moon#969). - if i + 1 + w >= args.len() { - return err("ERR syntax error"); - } - let wb = match extract_bytes(&args[i + 1 + w]) { - Some(b) => b, - None => return err("ERR syntax error"), - }; - // `"nan"` PARSES in Rust where C's `strtod` + `isnan` check in - // `getDoubleFromObjectOrReply` rejects it (moon#969), so a NaN - // weight sailed through and poisoned every aggregated score. - // Infinities stay legal, as they are on Redis. - let wval: f64 = match std::str::from_utf8(wb) - .ok() - .and_then(|s| s.parse::().ok()) - .filter(|v| !v.is_nan()) - { - Some(v) => v, - None => return err("ERR weight value is not a float"), - }; - weights[w] = wval; - } - i += 1 + numkeys; - } else if opt.eq_ignore_ascii_case(b"AGGREGATE") { - if i + 1 >= args.len() { - return err("ERR syntax error"); - } - let agg_b = match extract_bytes(&args[i + 1]) { - Some(b) => b.as_ref(), - None => return err("ERR syntax error"), - }; - aggregate = if agg_b.eq_ignore_ascii_case(b"SUM") { - AggregateOp::Sum - } else if agg_b.eq_ignore_ascii_case(b"MIN") { - AggregateOp::Min - } else if agg_b.eq_ignore_ascii_case(b"MAX") { - AggregateOp::Max - } else { - return err("ERR syntax error"); - }; - i += 2; - } else { - // moon#967 rewrote every OTHER zset option loop to reject an - // unrecognised token and missed this one, so `ZUNIONSTORE d 1 k - // BOGUS` stepped over `BOGUS` and answered a DIFFERENT, successful - // command. Redis: `ERR syntax error`. - return err("ERR syntax error"); - } - } - - // Read all source sets into a temporary structure - let mut source_data: Vec> = Vec::with_capacity(numkeys); - for key in &source_keys { - match db.get_sorted_set(key) { - Ok(Some((members, _))) => { - source_data.push(members.clone()); - } - Ok(None) => { - source_data.push(HashMap::new()); - } - Err(e) => return e, - } - } - - // Compute result - let mut result_map: HashMap = HashMap::new(); - - if intersect { - // Start with first set's members - if let Some(first) = source_data.first() { - for (member, score) in first { - let weighted = clamp_nan_to_zero(*score * weights[0]); - let mut final_score = weighted; - let mut in_all = true; - - for (idx, src) in source_data.iter().enumerate().skip(1) { - match src.get(member) { - Some(s) => { - let ws = clamp_nan_to_zero(*s * weights[idx]); - final_score = match aggregate { - AggregateOp::Sum => clamp_nan_to_zero(final_score + ws), - AggregateOp::Min => final_score.min(ws), - AggregateOp::Max => final_score.max(ws), - }; - } - None => { - in_all = false; - break; - } - } - } - - if in_all { - result_map.insert(member.clone(), final_score); - } - } - } - } else { - // Union: all members from all sets - for (idx, src) in source_data.iter().enumerate() { - for (member, score) in src { - let weighted = clamp_nan_to_zero(*score * weights[idx]); - result_map - .entry(member.clone()) - .and_modify(|existing| { - *existing = match aggregate { - AggregateOp::Sum => clamp_nan_to_zero(*existing + weighted), - AggregateOp::Min => existing.min(weighted), - AggregateOp::Max => existing.max(weighted), - }; - }) - .or_insert(weighted); - } - } - } - - let result_size = result_map.len() as i64; - - // Remove destination key first, then create new sorted set - db.remove(dest); - - if !result_map.is_empty() { - let (members, scores) = match db.get_or_create_sorted_set(dest) { - Ok(pair) => pair, - Err(e) => return e, - }; - - // `dest` was just removed/recreated above, so every member here is - // new -- charge each unconditionally (O(1) per member, no full - // recompute of the destination sorted set). - let mut mem_charge: usize = 0; - let table_before = zset_table_bytes(members, scores); - for (member, score) in result_map { - mem_charge += zset_member_cost(&member); - zadd_member(members, scores, member, score); - } - let table_after = zset_table_bytes(members, scores); - // `members`/`scores`' borrow of `db` ends above. - db.charge_memory(mem_charge); - db.adjust_memory(table_before, table_after); - } - - Frame::Integer(result_size) -} - -// --------------------------------------------------------------------------- -// ZRANGESTORE dst src min max [BYSCORE | BYLEX] [REV] [LIMIT offset count] -// --------------------------------------------------------------------------- - -/// ZRANGESTORE dst src min max [BYSCORE | BYLEX] [REV] [LIMIT offset count] -/// -/// Stores the result of a ZRANGE into `dst`, replacing it. Returns the cardinality of `dst`. -pub fn zrangestore(db: &mut Database, args: &[Frame]) -> Frame { - if args.len() < 4 { - return err_wrong_args("ZRANGESTORE"); - } - let dst = match extract_bytes(&args[0]) { - Some(k) => k, - None => return err_wrong_args("ZRANGESTORE"), - }; - let src = match extract_bytes(&args[1]) { - Some(k) => k, - None => return err_wrong_args("ZRANGESTORE"), - }; - let min_arg = match extract_bytes(&args[2]) { - Some(b) => b.clone(), - None => return err_wrong_args("ZRANGESTORE"), - }; - let max_arg = match extract_bytes(&args[3]) { - Some(b) => b.clone(), - None => return err_wrong_args("ZRANGESTORE"), - }; - - // Parse optional flags (same as ZRANGE but no WITHSCORES) - let mut by_score = false; - let mut by_lex = false; - let mut rev = false; - let mut limit_offset: Option = None; - let mut limit_count: Option = None; - - let mut i = 4; - while i < args.len() { - let opt = match extract_bytes(&args[i]) { - Some(b) => b.as_ref(), - None => { - i += 1; - continue; - } - }; - if opt.eq_ignore_ascii_case(b"BYSCORE") { - by_score = true; - i += 1; - } else if opt.eq_ignore_ascii_case(b"BYLEX") { - by_lex = true; - i += 1; - } else if opt.eq_ignore_ascii_case(b"REV") { - rev = true; - i += 1; - } else if opt.eq_ignore_ascii_case(b"LIMIT") { - if i + 2 < args.len() { - let off_b = match extract_bytes(&args[i + 1]) { - Some(b) => b, - None => return err_wrong_args("ZRANGESTORE"), - }; - let cnt_b = match extract_bytes(&args[i + 2]) { - Some(b) => b, - None => return err_wrong_args("ZRANGESTORE"), - }; - limit_offset = std::str::from_utf8(off_b).ok().and_then(|s| s.parse().ok()); - limit_count = std::str::from_utf8(cnt_b).ok().and_then(|s| s.parse().ok()); - if limit_offset.is_none() || limit_count.is_none() { - return err("ERR value is not an integer or out of range"); - } - i += 3; - } else { - return err_wrong_args("ZRANGESTORE"); - } - } else { - return err("ERR syntax error"); - } - } - - if by_score && by_lex { - return err("ERR BYSCORE and BYLEX options are not compatible"); - } - if limit_offset.is_some() && !by_score && !by_lex { - return err( - "ERR syntax error, LIMIT is only supported in combination with either BYSCORE or BYLEX", - ); - } - - // Run ZRANGE on src, collecting (member, score) pairs - let entries: Vec<(Bytes, f64)> = match db.get_sorted_set(src) { - Ok(Some((members, scores))) => { - let frame = if by_score { - zrange_by_score( - members, - scores, - &min_arg, - &max_arg, - rev, - true, - limit_offset, - limit_count, - ) - } else if by_lex { - zrange_by_lex( - scores, - &min_arg, - &max_arg, - rev, - true, - members, - limit_offset, - limit_count, - ) - } else { - zrange_by_rank(scores, &min_arg, &max_arg, rev, true) - }; - // Parse the Frame::Array([member, score, member, score, ...]) into Vec<(Bytes, f64)> - match frame { - Frame::Array(arr) => { - let mut result = Vec::with_capacity(arr.len() / 2); - let mut idx = 0; - while idx + 1 < arr.len() { - if let (Frame::BulkString(m), Frame::BulkString(s)) = - (&arr[idx], &arr[idx + 1]) - { - if let Ok(score) = std::str::from_utf8(s).unwrap_or("0").parse::() - { - result.push((m.clone(), score)); - } - } - idx += 2; - } - result - } - Frame::Error(_) => return frame, - _ => Vec::with_capacity(0), - } - } - Ok(None) => Vec::with_capacity(0), - Err(e) => return e, - }; - - let count = entries.len() as i64; - - // Replace dst with the result - db.remove(dst); - - if !entries.is_empty() { - let (dst_members, dst_scores) = match db.get_or_create_sorted_set(dst) { - Ok(pair) => pair, - Err(e) => return e, - }; - // `dst` was just removed/recreated above, so every entry is new. - let mut mem_charge: usize = 0; - let table_before = zset_table_bytes(dst_members, dst_scores); - for (member, score) in entries { - mem_charge += zset_member_cost(&member); - zadd_member(dst_members, dst_scores, member, score); - } - let table_after = zset_table_bytes(dst_members, dst_scores); - // `dst_members`/`dst_scores`' borrow of `db` ends above. - db.charge_memory(mem_charge); - db.adjust_memory(table_before, table_after); - } - - Frame::Integer(count) -} - // --------------------------------------------------------------------------- // ZMPOP numkeys key [key ...] MIN|MAX [COUNT n] // --------------------------------------------------------------------------- diff --git a/tests/zset_read_cold_tier_928.rs b/tests/zset_read_cold_tier_928.rs index 2e64e353e..bceadf622 100644 --- a/tests/zset_read_cold_tier_928.rs +++ b/tests/zset_read_cold_tier_928.rs @@ -73,6 +73,8 @@ fn read(db: &mut Database, name: &str, args: &[&[u8]]) -> Frame { "ZREVRANGE" => sorted_set::zrevrange(db, &f), "ZRANGEBYSCORE" => sorted_set::zrangebyscore(db, &f), "ZREVRANGEBYSCORE" => sorted_set::zrevrangebyscore(db, &f), + "ZRANGEBYLEX" => sorted_set::zrangebylex(db, &f), + "ZREVRANGEBYLEX" => sorted_set::zrevrangebylex(db, &f), "ZCOUNT" => sorted_set::zcount(db, &f), "ZLEXCOUNT" => sorted_set::zlexcount(db, &f), "ZMSCORE" => sorted_set::zmscore(db, &f), @@ -102,6 +104,10 @@ fn reads() -> Vec<(&'static str, Vec<&'static [u8]>)> { vec![&b"z"[..], b"-inf", b"+inf", b"WITHSCORES"], ), ("ZREVRANGEBYSCORE", vec![&b"z"[..], b"+inf", b"-inf"]), + // moon#959: the two lex reads share the helpers above and must answer + // a cold zset the same way. + ("ZRANGEBYLEX", vec![&b"z"[..], b"-", b"+"]), + ("ZREVRANGEBYLEX", vec![&b"z"[..], b"+", b"[b"]), ("ZCOUNT", vec![&b"z"[..], b"(1", b"3"]), ("ZLEXCOUNT", vec![&b"z"[..], b"[b", b"+"]), ("ZMSCORE", vec![&b"z"[..], b"a", b"nope", b"c"]), From febbd8a802c885a2572929eb1aa88dd0f72554b6 Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 16 Sep 2026 22:48:28 +0700 Subject: [PATCH 5/7] test(sorted_set): harness rows, docs correction and CHANGELOG for moon#959 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither scripts/test-commands.sh (504 rows) nor scripts/test-consistency.sh (132 rows) named any of the six commands or ZADD INCR, which is how a command docs/commands.md advertised went missing without a harness noticing. 29 rows join the first script and 87 the second, every one a reply read off redis 8.6.1: the happy paths, the bounds grammar (checked before the key), LIMIT, the numkeys classes and the refused option tokens of ZDIFFSTORE, WRONGTYPE from every command without clobbering the value, drained keys deleted, and the NX/XX/GT/LT decision order of ZADD INCR. Run verbatim against the pre-fix binary a8eb2efc: 109 of 116 rows red (62 of them `unknown command`, the rest the arity error ZADD INCR used to give); the 7 green are the fence rows that hold either way. Exclusive/inclusive bound tokens are quoted (`'(d'`): a bare `(` is a bash syntax error that kills the sourced script at that line, which the runner caught on the first attempt. docs/commands.md: the sorted-set line now lists exactly what dispatch accepts on a live server — the six new commands plus nine that were implemented but unlisted (ZRANGESTORE, ZDIFF, ZUNION, ZINTER, ZINTERCARD, ZMSCORE, ZRANDMEMBER, ZMPOP, BZMPOP), each sent to the server to confirm. Refs moon#959. author: Tin Dang --- CHANGELOG.md | 25 ++++++++ docs/commands.md | 7 ++- scripts/test-commands.sh | 38 ++++++++++++ scripts/test-consistency.sh | 114 ++++++++++++++++++++++++++++++++++++ 4 files changed, 182 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89f1a4ad1..112021840 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **Six sorted-set commands that were `unknown command`, and `ZADD ... INCR`** + (moon#959). `ZRANGEBYLEX`, `ZREVRANGEBYLEX`, `ZREMRANGEBYRANK`, + `ZREMRANGEBYSCORE`, `ZREMRANGEBYLEX` and `ZDIFFSTORE` are implemented, wired + into every dispatch path, registered as `@sortedset`, and covered by rows in + both parity harnesses; `docs/commands.md` had advertised `ZRANGEBYLEX` while + dispatch rejected it. `ZADD ... INCR` — which `redis-py`'s `zadd(..., + incr=True)` sends — replies the new score as a bulk string, or nil when + `NX`/`XX`/`GT`/`LT` refuse, in Redis's decision order. Every reply, error + surface included, was read off redis-server 8.6.1 before the code was + written: the range grammar is checked before the key is consulted, a + `ZREMRANGEBY*` that drains a key deletes it, a listpack zset is trimmed in + place and never converted, and the `used_memory` ledger stays exact on both + encodings. `ZDIFFSTORE` joins the `ZUNIONSTORE` family's `numkeys` and + option rules, refusing `WEIGHTS`/`AGGREGATE` as `syntax error`. + ### Changed - **BEHAVIOUR CHANGE — `ZADD ... GT LT` and a NaN `WEIGHTS` value now error** @@ -19,6 +36,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`ZUNIONSTORE`/`ZINTERSTORE` report `WRONGTYPE` before an option error, and + no longer flatten a listpack source** (moon#959). Redis looks every source up + before it parses `WEIGHTS`/`AGGREGATE`, so `ZUNIONSTORE d 1 + BOGUS` is `WRONGTYPE` on redis 8.6.1; moon answered `syntax error`. The store + family also read its sources through the promoting accessor, converting a + `listpack` source to `skiplist` as a side effect of reading it — the moon#928 + defect the read-only set operations were already cured of. Both fixes came + with the shared implementation `ZDIFFSTORE` now uses. - **Sorted-set argument validation reports the error CLASS Redis reports** (moon#969). Nine forms answered the wrong class, which matters beyond wording: redis-py raises a distinct exception type per class, so a client branching on diff --git a/docs/commands.md b/docs/commands.md index 35dded015..fe15a6921 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -47,9 +47,12 @@ Per-field return code on the TTL commands: `-2` = no such field, `-1` = no TTL, `SADD`, `SREM`, `SMEMBERS`, `SCARD`, `SISMEMBER`, `SMISMEMBER`, `SINTER`, `SUNION`, `SDIFF`, `SINTERSTORE`, `SUNIONSTORE`, `SDIFFSTORE`, `SRANDMEMBER`, `SPOP`, `SSCAN` -## Sorted sets (21) +## Sorted sets (35) -`ZADD`, `ZREM`, `ZSCORE`, `ZCARD`, `ZINCRBY`, `ZRANK`, `ZREVRANK`, `ZPOPMIN`, `ZPOPMAX`, `ZSCAN`, `ZRANGE`, `ZREVRANGE`, `ZRANGEBYSCORE`, `ZREVRANGEBYSCORE`, `ZRANGEBYLEX`, `ZCOUNT`, `ZLEXCOUNT`, `ZUNIONSTORE`, `ZINTERSTORE`, `BZPOPMIN`, `BZPOPMAX` +`ZADD`, `ZREM`, `ZSCORE`, `ZCARD`, `ZINCRBY`, `ZRANK`, `ZREVRANK`, `ZPOPMIN`, `ZPOPMAX`, `ZSCAN`, `ZRANGE`, `ZREVRANGE`, `ZRANGEBYSCORE`, `ZREVRANGEBYSCORE`, `ZRANGEBYLEX`, `ZREVRANGEBYLEX`, `ZCOUNT`, `ZLEXCOUNT`, `ZREMRANGEBYRANK`, `ZREMRANGEBYSCORE`, `ZREMRANGEBYLEX`, `ZUNIONSTORE`, `ZINTERSTORE`, `ZDIFFSTORE`, `ZRANGESTORE`, `ZDIFF`, `ZUNION`, `ZINTER`, `ZINTERCARD`, `ZMSCORE`, `ZRANDMEMBER`, `ZMPOP`, `BZPOPMIN`, `BZPOPMAX`, `BZMPOP` + +!!! tip + `ZADD` supports `NX`, `XX`, `GT`, `LT`, `CH` and `INCR`, matching Redis 6.2+ behavior. Every command in this list is accepted by dispatch on a live server — the table is checked against `scripts/test-consistency.sh`, not against `COMMAND INFO`. ## Geospatial (8) diff --git a/scripts/test-commands.sh b/scripts/test-commands.sh index 1bb6c38ae..ca7844ceb 100755 --- a/scripts/test-commands.sh +++ b/scripts/test-commands.sh @@ -951,6 +951,44 @@ if should_run "sorted_set"; then rcli ZADD z:ch 1 m >/dev/null 2>&1; mcli ZADD z:ch 1 m >/dev/null 2>&1 assert_match "ZADD CH sub-epsilon" ZADD z:ch CH 1.0000000000000002 m assert_match "ZADD CH moved the score" ZSCORE z:ch m + + # moon#959 -- six commands that answered `ERR unknown command` on moon + # (ZRANGEBYLEX, ZREVRANGEBYLEX, ZREMRANGEBYRANK, ZREMRANGEBYSCORE, + # ZREMRANGEBYLEX, ZDIFFSTORE) plus `ZADD ... INCR`, which answered an + # arity error. Neither harness named any of them, which is how a command + # docs/commands.md advertised went missing. Every reply was read off + # redis 8.6.1 before the commands were written. + rcli ZADD z:959:lex 0 a 0 b 0 c 0 d 0 e >/dev/null 2>&1; mcli ZADD z:959:lex 0 a 0 b 0 c 0 d 0 e >/dev/null 2>&1 + assert_match "ZRANGEBYLEX" ZRANGEBYLEX z:959:lex - + + assert_match "ZRANGEBYLEX bounds" ZRANGEBYLEX z:959:lex '[b' '(d' + assert_match "ZRANGEBYLEX LIMIT" ZRANGEBYLEX z:959:lex - + LIMIT 1 2 + assert_match "ZRANGEBYLEX bad bound" ZRANGEBYLEX z:959:lex a b + assert_match "ZRANGEBYLEX WITHSCORES" ZRANGEBYLEX z:959:lex - + WITHSCORES + assert_match "ZREVRANGEBYLEX" ZREVRANGEBYLEX z:959:lex + - + assert_match "ZREVRANGEBYLEX bounds" ZREVRANGEBYLEX z:959:lex '(d' '[b' LIMIT 0 1 + rcli ZADD z:959:r 1 a 2 b 3 c 4 d 5 e >/dev/null 2>&1; mcli ZADD z:959:r 1 a 2 b 3 c 4 d 5 e >/dev/null 2>&1 + assert_match "ZREMRANGEBYRANK" ZREMRANGEBYRANK z:959:r 0 0 + assert_match "ZREMRANGEBYRANK neg stop" ZREMRANGEBYRANK z:959:r -10 -6 + assert_match "ZREMRANGEBYSCORE" ZREMRANGEBYSCORE z:959:r '(2' 3 + assert_match "ZREMRANGEBYSCORE bad" ZREMRANGEBYSCORE z:959:r nan 1 + assert_match "ZREMRANGEBYLEX" ZREMRANGEBYLEX z:959:lex '[b' '(d' + assert_match "ZREMRANGEBYLEX arity" ZREMRANGEBYLEX z:959:lex - + x + assert_match "ZRANGE after ZREMRANGE" ZRANGE z:959:r 0 -1 WITHSCORES + assert_match "ZREMRANGEBYSCORE drains" ZREMRANGEBYSCORE z:959:r -inf +inf + assert_match "ZREMRANGE drained key" EXISTS z:959:r + assert_match "ZDIFFSTORE" ZDIFFSTORE {z}:diff 2 {z}:A {z}:B + assert_match "ZDIFFSTORE result" ZRANGE {z}:diff 0 -1 WITHSCORES + assert_match "ZDIFFSTORE numkeys 0" ZDIFFSTORE {z}:diff 0 {z}:A + assert_match "ZDIFFSTORE WEIGHTS" ZDIFFSTORE {z}:diff 1 {z}:A WEIGHTS 1 + assert_match "ZDIFFSTORE empty deletes" ZDIFFSTORE {z}:diff 2 {z}:A {z}:A + assert_match "ZDIFFSTORE dest gone" EXISTS {z}:diff + assert_match "ZADD INCR" ZADD z:959:i INCR 5 a + assert_match "ZADD INCR again" ZADD z:959:i INCR 2.5 a + assert_match "ZADD NX INCR refused" ZADD z:959:i NX INCR 1 a + assert_match "ZADD XX INCR refused" ZADD z:959:i XX INCR 1 nope + assert_match "ZADD GT INCR refused" ZADD z:959:i GT INCR -1 a + assert_match "ZADD LT INCR" ZADD z:959:i LT INCR -1 a + assert_match "ZADD INCR two pairs" ZADD z:959:i INCR 1 a 2 b fi # =========================================================================== diff --git a/scripts/test-consistency.sh b/scripts/test-consistency.sh index afd600738..5e731d759 100755 --- a/scripts/test-consistency.sh +++ b/scripts/test-consistency.sh @@ -952,6 +952,120 @@ both ZADD z:792:bt 1 bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb assert_both "ZADD CH sub-epsilon (bptree)" ZADD z:792:bt CH 1.0000000000000002 bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb assert_both "ZADD CH bptree moved score" ZSCORE z:792:bt bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb +# moon#959 -- six commands that answered `ERR unknown command` on moon +# (ZRANGEBYLEX, ZREVRANGEBYLEX, ZREMRANGEBYRANK, ZREMRANGEBYSCORE, +# ZREMRANGEBYLEX, ZDIFFSTORE) plus `ZADD ... INCR`, which answered an arity +# error. Every reply was read off redis 8.6.1 before the commands were +# written, error surface included: the bounds grammar is checked BEFORE the +# key (a bad bound on a missing key is an error, not an empty array), a +# drained key is deleted, and WRONGTYPE never clobbers the value it refused. +both ZADD z:959:lex 0 a 0 b 0 c 0 d 0 e +assert_both "ZRANGEBYLEX all" ZRANGEBYLEX z:959:lex - + +assert_both "ZRANGEBYLEX [b (d" ZRANGEBYLEX z:959:lex '[b' '(d' +assert_both "ZRANGEBYLEX LIMIT 1 2" ZRANGEBYLEX z:959:lex - + LIMIT 1 2 +assert_both "ZRANGEBYLEX LIMIT -1 2" ZRANGEBYLEX z:959:lex - + LIMIT -1 2 +assert_both "ZRANGEBYLEX reversed bounds" ZRANGEBYLEX z:959:lex + - +assert_both "ZRANGEBYLEX bad bound" ZRANGEBYLEX z:959:lex a b +assert_both "ZRANGEBYLEX bad bound missing key" ZRANGEBYLEX z:959:nokey a b +assert_both "ZRANGEBYLEX WITHSCORES" ZRANGEBYLEX z:959:lex - + WITHSCORES +assert_both "ZRANGEBYLEX dangling LIMIT" ZRANGEBYLEX z:959:lex - + LIMIT 1 +assert_both "ZRANGEBYLEX LIMIT notanint" ZRANGEBYLEX z:959:lex - + LIMIT notanint 1 +assert_both "ZRANGEBYLEX unknown token" ZRANGEBYLEX z:959:lex - + BOGUS +assert_both "ZRANGEBYLEX missing key" ZRANGEBYLEX z:959:nokey - + +assert_both "ZREVRANGEBYLEX all" ZREVRANGEBYLEX z:959:lex + - +assert_both "ZREVRANGEBYLEX (d [b" ZREVRANGEBYLEX z:959:lex '(d' '[b' +assert_both "ZREVRANGEBYLEX LIMIT" ZREVRANGEBYLEX z:959:lex + - LIMIT 1 2 +assert_both "ZREVRANGEBYLEX reversed bounds" ZREVRANGEBYLEX z:959:lex - + +both ZADD z:959:rank 1 a 2 b 3 c 4 d 5 e +assert_both "ZREMRANGEBYRANK 0 0" ZREMRANGEBYRANK z:959:rank 0 0 +# A stop still negative after normalisation is NOT clamped to 0 -- nothing +# is removed. (ZRANGE's own helper clamps it; that divergence is out of +# moon#959's scope and is reported separately.) +assert_both "ZREMRANGEBYRANK -10 -6" ZREMRANGEBYRANK z:959:rank -10 -6 +assert_both "ZREMRANGEBYRANK 3 1" ZREMRANGEBYRANK z:959:rank 3 1 +assert_both "ZREMRANGEBYRANK 1 -2" ZREMRANGEBYRANK z:959:rank 1 -2 +assert_both "ZREMRANGEBYRANK left" ZRANGE z:959:rank 0 -1 WITHSCORES +assert_both "ZREMRANGEBYRANK notanint" ZREMRANGEBYRANK z:959:rank notanint 1 +assert_both "ZREMRANGEBYRANK arity" ZREMRANGEBYRANK z:959:rank 1 +assert_both "ZREMRANGEBYRANK missing key" ZREMRANGEBYRANK z:959:nokey 0 1 +assert_both "ZREMRANGEBYRANK drains" ZREMRANGEBYRANK z:959:rank 0 -1 +assert_both "ZREMRANGEBYRANK drained key gone" EXISTS z:959:rank +both ZADD z:959:score 1 a 2 b 3 c 4 d 5 e +assert_both "ZREMRANGEBYSCORE (2 3" ZREMRANGEBYSCORE z:959:score '(2' 3 +assert_both "ZREMRANGEBYSCORE 3 1" ZREMRANGEBYSCORE z:959:score 3 1 +assert_both "ZREMRANGEBYSCORE 5 inf" ZREMRANGEBYSCORE z:959:score 5 inf +assert_both "ZREMRANGEBYSCORE left" ZRANGE z:959:score 0 -1 WITHSCORES +assert_both "ZREMRANGEBYSCORE nan" ZREMRANGEBYSCORE z:959:score nan 1 +assert_both "ZREMRANGEBYSCORE bad on missing" ZREMRANGEBYSCORE z:959:nokey a 1 +assert_both "ZREMRANGEBYSCORE drains" ZREMRANGEBYSCORE z:959:score -inf +inf +assert_both "ZREMRANGEBYSCORE drained key gone" EXISTS z:959:score +both ZADD z:959:lex2 0 a 0 b 0 c 0 d 0 e +assert_both "ZREMRANGEBYLEX [b (d" ZREMRANGEBYLEX z:959:lex2 '[b' '(d' +assert_both "ZREMRANGEBYLEX (c +" ZREMRANGEBYLEX z:959:lex2 '(c' + +assert_both "ZREMRANGEBYLEX left" ZRANGE z:959:lex2 0 -1 +assert_both "ZREMRANGEBYLEX bad bound" ZREMRANGEBYLEX z:959:lex2 a b +assert_both "ZREMRANGEBYLEX arity" ZREMRANGEBYLEX z:959:lex2 - + x +assert_both "ZREMRANGEBYLEX drains" ZREMRANGEBYLEX z:959:lex2 - + +assert_both "ZREMRANGEBYLEX drained key gone" EXISTS z:959:lex2 +both SET z:959:str v +assert_both "ZRANGEBYLEX WRONGTYPE" ZRANGEBYLEX z:959:str - + +assert_both "ZREVRANGEBYLEX WRONGTYPE" ZREVRANGEBYLEX z:959:str + - +assert_both "ZREMRANGEBYRANK WRONGTYPE" ZREMRANGEBYRANK z:959:str 0 1 +assert_both "ZREMRANGEBYSCORE WRONGTYPE" ZREMRANGEBYSCORE z:959:str 0 1 +assert_both "ZREMRANGEBYLEX WRONGTYPE" ZREMRANGEBYLEX z:959:str - + +assert_both "WRONGTYPE left the string" GET z:959:str +# ZDIFFSTORE joins the ZUNIONSTORE family: the same two numkeys classes, the +# same overrun rule, and EVERY option token refused (it takes none). Redis +# looks the sources up before it parses the options, so WRONGTYPE outranks +# an option error on all three STORE commands. `{z959}` co-locates the +# destination with its sources (moon#592). +both ZADD {z959}:a 1 a 2 b 3 c 4 d 5 e +both ZADD {z959}:b 1 a 2 b +both ZADD {z959}:c 2 b 9 x +both SET {z959}:str v +assert_both "ZDIFFSTORE two sources" ZDIFFSTORE {z959}:diff 2 {z959}:a {z959}:b +assert_both "ZDIFFSTORE result" ZRANGE {z959}:diff 0 -1 WITHSCORES +assert_both "ZDIFFSTORE three sources" ZDIFFSTORE {z959}:diff 3 {z959}:a {z959}:b {z959}:c +assert_both "ZDIFFSTORE result 3" ZRANGE {z959}:diff 0 -1 WITHSCORES +assert_both "ZDIFFSTORE missing first source" ZDIFFSTORE {z959}:diff 2 {z959}:nokey {z959}:a +assert_both "ZDIFFSTORE empty deletes dest" EXISTS {z959}:diff +assert_both "ZDIFFSTORE dest is a source" ZDIFFSTORE {z959}:c 2 {z959}:a {z959}:c +assert_both "ZDIFFSTORE dest-as-source result" ZRANGE {z959}:c 0 -1 WITHSCORES +assert_both "ZDIFFSTORE numkeys 0" ZDIFFSTORE {z959}:diff 0 {z959}:a +assert_both "ZDIFFSTORE numkeys -1" ZDIFFSTORE {z959}:diff -1 {z959}:a +assert_both "ZDIFFSTORE numkeys notanint" ZDIFFSTORE {z959}:diff notanint {z959}:a +assert_both "ZDIFFSTORE numkeys overruns" ZDIFFSTORE {z959}:diff 2 {z959}:a +assert_both "ZDIFFSTORE WEIGHTS refused" ZDIFFSTORE {z959}:diff 1 {z959}:a WEIGHTS 1 +assert_both "ZDIFFSTORE AGGREGATE refused" ZDIFFSTORE {z959}:diff 1 {z959}:a AGGREGATE SUM +assert_both "ZDIFFSTORE unknown token" ZDIFFSTORE {z959}:diff 1 {z959}:a BOGUS +assert_both "ZDIFFSTORE arity" ZDIFFSTORE {z959}:diff 1 +assert_both "ZDIFFSTORE WRONGTYPE source" ZDIFFSTORE {z959}:diff 2 {z959}:a {z959}:str +assert_both "ZDIFFSTORE WRONGTYPE beats option" ZDIFFSTORE {z959}:diff 1 {z959}:str BOGUS +assert_both "ZUNIONSTORE WRONGTYPE beats option" ZUNIONSTORE {z959}:diff 1 {z959}:str BOGUS +assert_both "ZINTERSTORE WRONGTYPE beats WEIGHTS" ZINTERSTORE {z959}:diff 2 {z959}:a {z959}:str WEIGHTS 1 1 +assert_both "ZDIFFSTORE errors made no dest" EXISTS {z959}:diff +# ZADD ... INCR: ZINCRBY's arithmetic under ZADD's flags, the new score as +# a bulk string, nil when a flag refuses. +assert_both "ZADD INCR new member" ZADD z:959:incr INCR 5 a +assert_both "ZADD INCR existing" ZADD z:959:incr INCR 2.5 a +assert_both "ZADD NX INCR present" ZADD z:959:incr NX INCR 1 a +assert_both "ZADD NX INCR absent" ZADD z:959:incr NX INCR 1 n +assert_both "ZADD XX INCR absent" ZADD z:959:incr XX INCR 1 nope +assert_both "ZADD XX INCR present" ZADD z:959:incr XX INCR 1 a +assert_both "ZADD GT INCR refused" ZADD z:959:incr GT INCR -1 a +assert_both "ZADD GT INCR zero refused" ZADD z:959:incr GT INCR 0 a +assert_both "ZADD LT INCR" ZADD z:959:incr LT INCR -1 a +assert_both "ZADD XX GT INCR absent" ZADD z:959:incr XX GT INCR 1 q +assert_both "ZADD INCR CH" ZADD z:959:incr INCR CH 1 a +assert_both "ZADD INCR two pairs" ZADD z:959:incr INCR 1 a 2 b +assert_both "ZADD INCR odd tail" ZADD z:959:incr INCR 1 +assert_both "ZADD INCR nan" ZADD z:959:incr INCR nan a +assert_both "ZADD INCR inf" ZADD z:959:incr INCR inf a +assert_both "ZADD INCR inf + -inf" ZADD z:959:incr INCR -inf a +assert_both "ZADD INCR after refusals" ZRANGE z:959:incr 0 -1 WITHSCORES +assert_both "ZADD XX INCR on missing key" ZADD z:959:incr:xx XX INCR 1 a +assert_both "ZADD XX INCR made no key" EXISTS z:959:incr:xx + # Exactly zset-max-listpack-entries (128) members is STILL a listpack; one # more promotes to a skiplist on both. One ZADD per step, not 129 — each # `both` spawns two redis-cli processes. From 0d05221f58d9bc2a46e680a2276e52d1fb2edc77 Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 16 Sep 2026 22:48:29 +0700 Subject: [PATCH 6/7] fix(server): ZDIFFSTORE joins the cross-shard write guard (shared.rs, isolated) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ISOLATED COMMIT — the only change to src/server/conn/shared.rs in this branch, confined to `touches_a_key_it_did_not_route_on`'s doc block and its match. `is_inline_intercepted` and the `INTERCEPTED_NOT_DECLARED` waiver (moon#937 / moon#946, owned by a parallel change) are untouched. That doc block listed ZDIFFSTORE as "not implemented, so nothing to misplace" and named `tests/two_key_write_cross_shard.rs::t2k4` as the tripwire that fires the moment it starts working — which moon#959 did. Without the guard, `ZDIFFSTORE dst 1 src` at --shards >= 2 would ack +OK and write `dst` into the wrong shard's slice (the moon#592 defect). The arm is the same shape as ZUNIONSTORE/ZINTERSTORE's: routed on `dst`, reads every source. t2k4 migrates ZDIFFSTORE into PROBES, exactly as GEORADIUS/ GEORADIUSBYMEMBER migrated under moon#645, and keeps the two `_RO` rows. Refs moon#959, moon#592. author: Tin Dang --- src/server/conn/shared.rs | 17 +++++++------ tests/two_key_write_cross_shard.rs | 40 ++++++++++++++++++------------ 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/server/conn/shared.rs b/src/server/conn/shared.rs index 1430fde0b..6e8723c2b 100644 --- a/src/server/conn/shared.rs +++ b/src/server/conn/shared.rs @@ -2081,14 +2081,12 @@ pub(crate) const CROSS_SHARD_WRITE_ERROR: &[u8] = /// entry points (`blocking::immediate_scan`, `blocking::wakeup`) that this /// pre-routing guard cannot see. Two overlapping guards for one family would /// be worse than one complete one. -/// * `ZDIFFSTORE` — not implemented in moon (unknown command), so there is -/// no write to misplace, and claiming `CROSSSLOT` would send a client -/// chasing hash tags for a command that will never work. -/// `tests/two_key_write_cross_shard.rs::t2k4` fails the moment it starts -/// working, which is when it must be added here. `GEORADIUS`/ -/// `GEORADIUSBYMEMBER` used to sit in this same bucket; moon#645 -/// implemented their `STORE`/`STOREDIST` clause, so they moved INTO the -/// family below in the same change that made them able to write. +/// * `ZDIFFSTORE` sat in this list as "not implemented, so nothing to +/// misplace" until moon#959 implemented it; it moved INTO the family +/// below in the same change, exactly as `GEORADIUS`/`GEORADIUSBYMEMBER` +/// did when moon#645 gave them a `STORE`/`STOREDIST` clause. The +/// `tests/two_key_write_cross_shard.rs::t2k4` tripwire is what forces +/// that migration. /// * The read-only multi-key commands (`SINTER`, `SUNION`, `SDIFF`, `ZDIFF`, /// `ZINTER`, `ZUNION`, `ZINTERCARD`, `SINTERCARD`, `LCS`, `PFCOUNT`, /// `TOUCH`, `LMPOP`, `ZMPOP`) — same routing rule, but the consequence is a @@ -2118,6 +2116,9 @@ fn touches_a_key_it_did_not_route_on(cmd: &[u8]) -> bool { || cmd.eq_ignore_ascii_case(b"ZUNIONSTORE") || cmd.eq_ignore_ascii_case(b"ZINTERSTORE") } + // `ZDIFFSTORE dst numkeys src ...` (moon#959): routed on `dst`, reads + // every source — the same shape as its two siblings above. + (10, b'z') => cmd.eq_ignore_ascii_case(b"ZDIFFSTORE"), (7, b'p') => cmd.eq_ignore_ascii_case(b"PFMERGE"), (14, b'g') => cmd.eq_ignore_ascii_case(b"GEOSEARCHSTORE"), // `GEORADIUS src ... STORE|STOREDIST dst` (moon#645). Without the diff --git a/tests/two_key_write_cross_shard.rs b/tests/two_key_write_cross_shard.rs index ac3391303..6605ff4f6 100644 --- a/tests/two_key_write_cross_shard.rs +++ b/tests/two_key_write_cross_shard.rs @@ -214,6 +214,19 @@ const PROBES: &[Probe] = &[ src_untouched: ":2\r\n", src_after_success: None, }, + // moon#959 implemented ZDIFFSTORE, which is what moved it out of the + // t2k4 tripwire below and into the family. + Probe { + label: "ZDIFFSTORE", + seed: &[&["ZADD", "{s}", "1", "a", "2", "b"]], + argv: &["ZDIFFSTORE", "{d}", "1", "{s}"], + dst_probe: &["ZCARD", "{d}"], + dst_landed: ":2\r\n", + dst_absent: ":0\r\n", + src_probe: &["ZCARD", "{s}"], + src_untouched: ":2\r\n", + src_after_success: None, + }, Probe { label: "PFMERGE", seed: &[&["PFADD", "{s}", "a", "b", "c"]], @@ -549,23 +562,19 @@ fn t2k3_hash_tagged_pairs_still_work_at_four_shards() { ); } -/// Tripwire for the member of the family moon does not implement yet. -/// -/// `ZDIFFSTORE` is not in the dispatch table, so it cannot misplace a -/// destination today, which is the only reason it is absent from `PROBES` -/// and from the routing guard. +/// Tripwire for store forms moon does not implement (or must never accept). /// -/// If this test ever fails, it started working — and it went in WITHOUT a -/// cross-shard guard, which means it shipped the moon#592 defect. Add it to -/// `PROBES` and to `shared::touches_a_key_it_did_not_route_on`'s family list -/// in the same change. +/// If a row here ever answers a success, it started working — and it went in +/// WITHOUT a cross-shard guard, which means it shipped the moon#592 defect. +/// Add it to `PROBES` and to `shared::touches_a_key_it_did_not_route_on`'s +/// family list in the same change. /// -/// `GEORADIUS`/`GEORADIUSBYMEMBER ... STORE` were the other two rows here -/// until moon#645. They now work, so they are covered by `PROBES` above — -/// which is exactly the migration this tripwire exists to force. The `_RO` -/// twins still refuse the clause and are checked below, because a read-only -/// command that started writing would be a worse defect than the one this -/// file is about. +/// `GEORADIUS`/`GEORADIUSBYMEMBER ... STORE` were rows here until moon#645, +/// and `ZDIFFSTORE` until moon#959. They now work, so they are covered by +/// `PROBES` above — which is exactly the migration this tripwire exists to +/// force. The `_RO` twins still refuse the clause and are checked below, +/// because a read-only command that started writing would be a worse defect +/// than the one this file is about. #[test] fn t2k4_unimplemented_store_forms_stay_unimplemented_or_get_a_guard() { let m = spawn_moon(SHARDS); @@ -587,7 +596,6 @@ fn t2k4_unimplemented_store_forms_stay_unimplemented_or_get_a_guard() { assert_eq!(seeded, ":2\r\n", "GEOADD must seed {src}"); } let cases: &[(&str, &[&str])] = &[ - ("ZDIFFSTORE", &["ZDIFFSTORE", "t2k:zd:d", "1", "t2k:zd:s"]), ( "GEORADIUS_RO STORE", &[ From 10bd1ff08c643eeb4ff466971c5b1449e92ca5fc Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Wed, 16 Sep 2026 22:55:07 +0700 Subject: [PATCH 7/7] fix(sorted_set): ZRANGEBYLEX refuses WITHSCORES before it parses the bounds The first draft validated the lex bounds before the WITHSCORES refusal; the oracle sweep of the built binary (295 probes vs redis 8.6.1) caught the one probe that disagreed: `ZRANGEBYLEX k a b WITHSCORES` is `ERR syntax error, WITHSCORES not supported in combination with BYLEX` on redis, not the bound error. The unit test had encoded the wrong guess and is corrected; a row pinning the precedence joins both harnesses, plus one for the option loop still outranking WITHSCORES (`... WITHSCORES LIMIT 1` is a plain `syntax error`). Also: the registry comment no longer cites `COMMAND INFO` as a source (every arity was compared on the wire), and the docs tip states what was actually done to re-verify the sorted-set table. Refs moon#959. author: Tin Dang --- docs/commands.md | 2 +- scripts/test-commands.sh | 1 + scripts/test-consistency.sh | 2 ++ src/command/metadata.rs | 4 ++-- src/command/sorted_set/mod.rs | 6 ++++-- src/command/sorted_set/sorted_set_lex.rs | 21 ++++++++++++--------- 6 files changed, 22 insertions(+), 14 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index fe15a6921..888e94e62 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -52,7 +52,7 @@ Per-field return code on the TTL commands: `-2` = no such field, `-1` = no TTL, `ZADD`, `ZREM`, `ZSCORE`, `ZCARD`, `ZINCRBY`, `ZRANK`, `ZREVRANK`, `ZPOPMIN`, `ZPOPMAX`, `ZSCAN`, `ZRANGE`, `ZREVRANGE`, `ZRANGEBYSCORE`, `ZREVRANGEBYSCORE`, `ZRANGEBYLEX`, `ZREVRANGEBYLEX`, `ZCOUNT`, `ZLEXCOUNT`, `ZREMRANGEBYRANK`, `ZREMRANGEBYSCORE`, `ZREMRANGEBYLEX`, `ZUNIONSTORE`, `ZINTERSTORE`, `ZDIFFSTORE`, `ZRANGESTORE`, `ZDIFF`, `ZUNION`, `ZINTER`, `ZINTERCARD`, `ZMSCORE`, `ZRANDMEMBER`, `ZMPOP`, `BZPOPMIN`, `BZPOPMAX`, `BZMPOP` !!! tip - `ZADD` supports `NX`, `XX`, `GT`, `LT`, `CH` and `INCR`, matching Redis 6.2+ behavior. Every command in this list is accepted by dispatch on a live server — the table is checked against `scripts/test-consistency.sh`, not against `COMMAND INFO`. + `ZADD` supports `NX`, `XX`, `GT`, `LT`, `CH` and `INCR`, matching Redis 6.2+ behavior. Every command in this list was sent to a live moon server and answered (moon#959 re-verified the table command by command, not from `COMMAND INFO`, after it had advertised an unimplemented one). ## Geospatial (8) diff --git a/scripts/test-commands.sh b/scripts/test-commands.sh index ca7844ceb..a77eada85 100755 --- a/scripts/test-commands.sh +++ b/scripts/test-commands.sh @@ -964,6 +964,7 @@ if should_run "sorted_set"; then assert_match "ZRANGEBYLEX LIMIT" ZRANGEBYLEX z:959:lex - + LIMIT 1 2 assert_match "ZRANGEBYLEX bad bound" ZRANGEBYLEX z:959:lex a b assert_match "ZRANGEBYLEX WITHSCORES" ZRANGEBYLEX z:959:lex - + WITHSCORES + assert_match "ZRANGEBYLEX WITHSCORES 1st" ZRANGEBYLEX z:959:lex a b WITHSCORES assert_match "ZREVRANGEBYLEX" ZREVRANGEBYLEX z:959:lex + - assert_match "ZREVRANGEBYLEX bounds" ZREVRANGEBYLEX z:959:lex '(d' '[b' LIMIT 0 1 rcli ZADD z:959:r 1 a 2 b 3 c 4 d 5 e >/dev/null 2>&1; mcli ZADD z:959:r 1 a 2 b 3 c 4 d 5 e >/dev/null 2>&1 diff --git a/scripts/test-consistency.sh b/scripts/test-consistency.sh index 5e731d759..d530d575d 100755 --- a/scripts/test-consistency.sh +++ b/scripts/test-consistency.sh @@ -968,6 +968,8 @@ assert_both "ZRANGEBYLEX reversed bounds" ZRANGEBYLEX z:959:lex + - assert_both "ZRANGEBYLEX bad bound" ZRANGEBYLEX z:959:lex a b assert_both "ZRANGEBYLEX bad bound missing key" ZRANGEBYLEX z:959:nokey a b assert_both "ZRANGEBYLEX WITHSCORES" ZRANGEBYLEX z:959:lex - + WITHSCORES +assert_both "ZRANGEBYLEX WITHSCORES beats bound" ZRANGEBYLEX z:959:lex a b WITHSCORES +assert_both "ZRANGEBYLEX LIMIT beats WITHSCORES" ZRANGEBYLEX z:959:lex - + WITHSCORES LIMIT 1 assert_both "ZRANGEBYLEX dangling LIMIT" ZRANGEBYLEX z:959:lex - + LIMIT 1 assert_both "ZRANGEBYLEX LIMIT notanint" ZRANGEBYLEX z:959:lex - + LIMIT notanint 1 assert_both "ZRANGEBYLEX unknown token" ZRANGEBYLEX z:959:lex - + BOGUS diff --git a/src/command/metadata.rs b/src/command/metadata.rs index 0262a25a0..269f5c0eb 100644 --- a/src/command/metadata.rs +++ b/src/command/metadata.rs @@ -309,8 +309,8 @@ pub static COMMAND_META: phf::Map<&'static str, CommandMeta> = phf_map! { "ZMSCORE" => CommandMeta { name: "ZMSCORE", arity: -3, flags: RFP, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, "ZRANDMEMBER" => CommandMeta { name: "ZRANDMEMBER", arity: -2, flags: R, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, "ZMPOP" => CommandMeta { name: "ZMPOP", arity: -4, flags: W, first_key: 0, last_key: 0, step: 0, acl_categories: ZST }, - // moon#959. Arities and key specs transcribed from redis 8.6.1's - // `COMMAND INFO`; the replies themselves were verified on the wire. + // moon#959. Arities match redis 8.6.1 as SENT — every arity error below + // was compared on the wire, not read off `COMMAND INFO`. "ZRANGEBYLEX" => CommandMeta { name: "ZRANGEBYLEX", arity: -4, flags: R, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, "ZREVRANGEBYLEX" => CommandMeta { name: "ZREVRANGEBYLEX", arity: -4, flags: R, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, "ZREMRANGEBYRANK" => CommandMeta { name: "ZREMRANGEBYRANK", arity: 4, flags: W, first_key: 1, last_key: 1, step: 1, acl_categories: ZST }, diff --git a/src/command/sorted_set/mod.rs b/src/command/sorted_set/mod.rs index fc4d703f7..997e80961 100644 --- a/src/command/sorted_set/mod.rs +++ b/src/command/sorted_set/mod.rs @@ -4311,10 +4311,12 @@ mod missing_commands_959_tests { e(&mut db, cmd, &["lex", "-", "+", "WITHSCORES"]), "ERR syntax error, WITHSCORES not supported in combination with BYLEX" ); - // ... but a bad bound outranks the WITHSCORES refusal. + // ... and the WITHSCORES refusal outranks a bad bound. (The + // first draft had these the other way round; the oracle sweep of + // the built binary caught it, which is why every row is sent.) assert_eq!( e(&mut db, cmd, &["lex", "a", "b", "WITHSCORES"]), - "ERR min or max not valid string range item" + "ERR syntax error, WITHSCORES not supported in combination with BYLEX" ); assert_eq!( e(&mut db, cmd, &["lex", "-", "+", "LIMIT", "1"]), diff --git a/src/command/sorted_set/sorted_set_lex.rs b/src/command/sorted_set/sorted_set_lex.rs index f2a745b5c..9764db269 100644 --- a/src/command/sorted_set/sorted_set_lex.rs +++ b/src/command/sorted_set/sorted_set_lex.rs @@ -48,11 +48,13 @@ pub fn zrevrangebylex_readonly(db: &Database, args: &[Frame], now_ms: u64) -> Fr /// Error precedence follows Redis's `zrangeGenericCommand`, verified against /// redis-server 8.6.1: the option loop first (a dangling `LIMIT` or an unknown /// token is `syntax error`, a non-integer `LIMIT` value is the generic integer -/// error), then the range grammar (`min or max not valid string range item`), -/// then `WITHSCORES` — which the legacy spelling parses but refuses with its -/// own message — and only then the key. The bounds are therefore validated -/// BEFORE the lookup, so `ZRANGEBYLEX nokey a b` is an error and not an empty -/// array. +/// error), then `WITHSCORES` — which the legacy spelling parses but refuses +/// with its own message, BEFORE it looks at the bounds — then the range +/// grammar (`min or max not valid string range item`), and only then the key. +/// The bounds are therefore validated BEFORE the lookup, so `ZRANGEBYLEX +/// nokey a b` is an error and not an empty array. The first draft checked the +/// bounds before WITHSCORES; the oracle sweep of the built binary caught it — +/// `ZRANGEBYLEX k a b WITHSCORES` is the WITHSCORES error on redis. fn zrangebylex_impl(db: &Database, args: &[Frame], now_ms: u64, rev: bool) -> Frame { let cmd = if rev { "ZREVRANGEBYLEX" } else { "ZRANGEBYLEX" }; if args.len() < 3 { @@ -102,7 +104,8 @@ fn zrangebylex_impl(db: &Database, args: &[Frame], now_ms: u64, rev: bool) -> Fr } i += 3; } else if opt.eq_ignore_ascii_case(b"WITHSCORES") { - // Parsed here, refused below: the range grammar is checked first. + // Parsed here, refused below, after the whole option loop: a + // later dangling `LIMIT` still wins. withscores = true; i += 1; } else { @@ -110,6 +113,9 @@ fn zrangebylex_impl(db: &Database, args: &[Frame], now_ms: u64, rev: bool) -> Fr } } + if withscores { + return err("ERR syntax error, WITHSCORES not supported in combination with BYLEX"); + } // Validate the grammar before the key is consulted. The helpers below // parse the bounds again; that second pass is two small copies on a path // that is about to materialise the reply, and it keeps the ONE grammar @@ -120,9 +126,6 @@ fn zrangebylex_impl(db: &Database, args: &[Frame], now_ms: u64, rev: bool) -> Fr if let Err(e) = parse_lex_bound(max_arg) { return e; } - if withscores { - return err("ERR syntax error, WITHSCORES not supported in combination with BYLEX"); - } match db.get_sorted_set_ref_if_alive(key, now_ms) { Ok(Some(zref)) => match (zref.members_map(), zref.bptree()) {