Skip to content

Order lex range bounds the way Redis does (#1302) - #3334

Open
mohsinaliwork686-rgb wants to merge 5 commits into
StackExchange:mainfrom
mohsinaliwork686-rgb:fix/lex-range-numeric-prefix
Open

mohsinaliwork686-rgb wants to merge 5 commits into
StackExchange:mainfrom
mohsinaliwork686-rgb:fix/lex-range-numeric-prefix

Conversation

@mohsinaliwork686-rgb

@mohsinaliwork686-rgb mohsinaliwork686-rgb commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1302

SortedSetRangeByValue(key, "0123", "0123\xff") returned nothing, in both ascending and descending order, while the same ZRANGEBYLEX sent directly returned the matching members. Before sending the command, ReverseLimits puts the two bounds in order with RedisValue.CompareTo, and that reads "0123" as the integer 123. Compared against the text "0123\xff", it then came out as the larger of the two, so the bounds were swapped and Redis was asked for an empty range ("[0123\xc3\xbf" "[0123"). A prefix such as ABC was not affected.

As agreed on the issue, RedisValue.CompareTo is left alone and only the bound ordering changes. ReverseLimits now uses a new internal RedisValue.LexCompare, which compares the bounds as raw bytes, which is how Redis orders a lex range (memcmp order, with a shorter prefix first). When both bounds are byte-backed it calls BlobCompareTo, with no copies; otherwise it gets the bytes from AsMemory, the helper StartsWith already uses, leasing from ArrayPool instead of allocating. Like CompareTo, it never throws: if a value can't be read as bytes it returns 0, so ReverseLimits behaves as it did before for such values. The stop != default check and the Exclude swap are unchanged. SortedSetLengthByValue now orders its bounds the same way, so it counts what SortedSetRangeByValue returns. SortedSetRemoveRangeByValue still does not reorder: bounds the wrong way round remove nothing, as before, because reordering "2" / "10" would also delete "15" and "100", which a caller thinking of numbers never asked for. The XML docs on all three now describe this.

One side effect: bounds made only of digits are now ordered as text, the way Redis sees them. SortedSetRangeByValue(key, "2", "10") used to send [2 [10, which is always empty because "10" sorts before "2"; it now sends [10 [2 and returns the members in between. The same goes for integer RedisValue bounds, since they are sent as text, and for byte[] bounds, which CompareTo also read as numbers. SortedSetLengthByValue follows the range, so a count with its bounds the wrong way round ("2" / "10", or "g" / "a") now counts that range instead of returning 0. The order only changes when the bounds sort the wrong way round as bytes, and in that case the old command always asked Redis for an empty range, so every call that returned members, or a non-zero count, before sends the same command now.

Tests, in LexTests:

  • QueryRangeByLexWithNumericPrefix covers the "0123" / "0123\xff" case from the issue, ascending and descending, through both the sync and async methods.
  • QueryRangeByLexWithNumericPrefixAndExclude covers "0123" / "0123xyz" with Exclude.Start and Exclude.Stop, in both orders.
  • QueryRangeByLexWithDigitOnlyBounds checks "2" / "10" in both orders against what Redis returns for ZRANGEBYLEX key [10 [2 and ZREVRANGEBYLEX key [2 [10 sent directly.
  • QueryRangeByLexWithIntegerBounds does the same with integer bounds, RedisValue min = 2; RedisValue max = 10;.
  • QueryRangeByLexWithNumericPrefixAsBytes repeats the "0123" case with byte[] bounds holding the same UTF-8 bytes, and with a string bound against a byte[] bound.
  • LengthByLexMatchesRangeWithDigitOnlyBounds checks that SortedSetLengthByValue(key, "2", "10"), sync and async, equals the number of members SortedSetRangeByValue returns, including with Exclude.Start.
  • RemoveRangeByLexDoesNotReorderBounds checks that SortedSetRemoveRangeByValue(key, "2", "10") removes nothing and leaves the set as it was.

And in RedisValueEquivalencyUnitTests, which needs no server, LexCompare_OrdersByRawBytes and LexCompare_DoesNotDependOnHowAValueIsStored check that LexCompare puts "10" before "2" and 10 before 2, orders by UTF-8 bytes rather than UTF-16 chars, puts null and empty first, and gives the same answer for every storage kind (string, integer, double, ulong, short blob, byte[], multi-segment sequence and MemoryManager).

The five range tests fail on main, where each gets an empty result, and pass with the change. The length test fails on main and on the first commit of this PR, where ZLEXCOUNT still answered 0. The remove test passes everywhere, since it pins behaviour that has not changed.

Locally, against Redis 7.4 on net10.0: LexTests, the two classes that check key prefixing against a mock (KeyPrefixedDatabaseTests and KeyPrefixedTests, the only others that call these methods), and RedisValueEquivalencyUnitTests give 565 passed, 0 failed, 0 skipped. The library builds with no warnings on every target framework, and the public API files are unchanged.

Checklist

  • I fully and freely contribute this code in accordance with the project license (and am legally able to do so)
  • I take responsibility for this contribution's quality and correctness, including any portions produced with AI assistance (see CONTRIBUTING.md).

@mgravell

mgravell commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this — it's a well-researched fix for a long-standing annoyance (#1302), and the write-up is genuinely excellent: the root cause analysis is spot on, and the observation that the order only changes where the old command was always empty is exactly the right framing for why this is safe. The new tests failing on main and passing with the change is much appreciated too.

A few things came up in review — mostly follow-ups rather than problems with the core fix, which looks sound:

  1. The paired by-value APIs now disagree for the fixed inputs. You note (correctly) that SortedSetLengthByValue and SortedSetRemoveRangeByValue never reorder, so they're untouched — but that now means SortedSetRangeByValue(key, "2", "10") returns members while SortedSetLengthByValue(key, "2", "10") still sends ZLEXCOUNT key [2 [10 and answers 0, and SortedSetRemoveRangeByValue removes nothing. Before this change all three were consistently empty/0 for those inputs. Since they share GetLexMessage, ordering the bounds there instead of in ReverseLimits would keep the trio in agreement — unless there's a reason to think reordering Length/Remove is riskier (removal is destructive, so worth a deliberate decision either way).

  2. The old comparer could never throw; the new one can. RedisValue.CompareTo swallows exceptions and falls back to 0, so ReverseLimits previously couldn't fault. CompareLexBounds materializes both bounds via AsMemory, which has throwing paths (pool rent on a huge value, the TryFormat failure branch), so an exotic bound can now surface a new exception type from SortedSetRangeByValue before anything is sent. Probably fine in practice, but worth either a guard or a conscious "this is acceptable" note.

  3. Test coverage for non-string bounds. The PR body says integer RedisValue bounds get the same treatment (true by construction), but all three new tests use string bounds, so only the String branch of AsMemory runs through the comparer. One case with RedisValue min = 2; RedisValue max = 10; — and ideally a byte[]-backed bound — would pin the claim down against future changes to the formatting paths.

  4. Allocation note (optional, non-blocking). CompareLexBounds leases two ArrayPool buffers and copies on every call, even when both bounds are already byte-backed; RedisValue.BlobCompareTo already does memcmp-order comparison allocation-free via UnsafeRawSpan/RawSequence, and TryGetSpan exposes the no-copy path. A fast path for span-available values (or an internal byte-order comparer on RedisValue next to BlobCompareTo, keeping the storage-kind dispatch inside RedisValue) would avoid the copies. Happy to take this as a follow-up rather than hold the PR on it.

  5. Docs. Digit-only bounds going from "always empty" to "returns the members between" is user-visible; a line in the min/max remarks on IDatabase.SortedSetRangeByValue (bounds are ordered by raw bytes, the way Redis orders a lex range) — and, if the Length/Remove asymmetry stays, a note about that — would give users something to consult when they notice the change.

None of these undermine the fix itself — the swap logic traces out correctly for null/empty/equal/swapped bounds, and the lease handling in the finally block is right. Thanks again for the care that went into this.

@mohsinaliwork686-rgb

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. I've added new commits on top of 715184a rather than squashing, so the part you've already read is unchanged.

  1. Length and Remove (073892e). SortedSetLengthByValue now calls ReverseLimits the same way the range does, so it counts what SortedSetRangeByValue returns. SortedSetRemoveRangeByValue deliberately still doesn't reorder: it's destructive, a reversed pair removes nothing today, and reordering "2" / "10" would delete everything from "10" to "2" as text, "15" and "100" included, which someone thinking in numbers never meant. That's also why I left GetLexMessage alone. If you'd rather have all three agree, I'm happy to reorder Remove as well. One thing to be aware of: Length with plain-text bounds the wrong way round ("g" / "a") now counts that range instead of returning 0, which matches what Range already did for them.

  2. Throwing (25427cd). The new comparer catches any exception and returns 0, the same as CompareTo, so ReverseLimits still can't fault, and a value that can't be read as bytes is handled exactly as before.

  3. Non-string bounds (485491a). New LexTests: integer bounds 2 / 10 in both orders, checked against ZRANGEBYLEX / ZREVRANGEBYLEX sent directly; byte[] bounds for the "0123" prefix, plus one string-against-byte[] pair; Length matching Range for "2" / "10", including Exclude.Start; and Remove for "2" / "10" removing nothing. I also added server-free tests for the comparer in RedisValueEquivalencyUnitTests, covering every storage kind. Side note: the byte[] case fails on main too, because CompareTo also simplifies a byte-backed "0123" to a number.

  4. Copies (25427cd). I took your suggestion: there's now an internal RedisValue.LexCompare next to BlobCompareTo. When both values are byte-backed it calls BlobCompareTo, with no copies; otherwise it goes through AsMemory with pool leases, the same way StartsWith does. CompareLexBounds is gone and AsMemory is private again.

  5. Docs (f07737d). Both SortedSetRangeByValue overloads and SortedSetLengthByValue now have remarks saying the bounds are compared by raw bytes, the way Redis orders a lex range, and swapped if needed, so a numeric-looking bound is ordered as text. SortedSetRemoveRangeByValue says its bounds are not reordered and a reversed pair removes nothing. The async methods inherit these through inheritdoc.

Locally, against Redis 7.4 on net10.0, LexTests, KeyPrefixedDatabaseTests, KeyPrefixedTests and RedisValueEquivalencyUnitTests all pass (565 tests), and the build has no warnings on any target framework. I've updated the PR description to match.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SortedSetRangeByValue ReverseLimits breaks autocomplete numeric 0 prefix

2 participants