Repository navigation
Order lex range bounds the way Redis does (#1302) - #3334
mohsinaliwork686-rgb wants to merge 5 commits into
Conversation
|
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 A few things came up in review — mostly follow-ups rather than problems with the core fix, which looks sound:
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 |
…e-backed values and no throwing
…ave removal strict
…and LexCompare itself
|
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.
Locally, against Redis 7.4 on net10.0, |
Fixes #1302
SortedSetRangeByValue(key, "0123", "0123\xff")returned nothing, in both ascending and descending order, while the sameZRANGEBYLEXsent directly returned the matching members. Before sending the command,ReverseLimitsputs the two bounds in order withRedisValue.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 asABCwas not affected.As agreed on the issue,
RedisValue.CompareTois left alone and only the bound ordering changes.ReverseLimitsnow uses a new internalRedisValue.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 callsBlobCompareTo, with no copies; otherwise it gets the bytes fromAsMemory, the helperStartsWithalready uses, leasing fromArrayPoolinstead of allocating. LikeCompareTo, it never throws: if a value can't be read as bytes it returns 0, soReverseLimitsbehaves as it did before for such values. Thestop != defaultcheck and theExcludeswap are unchanged.SortedSetLengthByValuenow orders its bounds the same way, so it counts whatSortedSetRangeByValuereturns.SortedSetRemoveRangeByValuestill 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 [2and returns the members in between. The same goes for integerRedisValuebounds, since they are sent as text, and forbyte[]bounds, whichCompareToalso read as numbers.SortedSetLengthByValuefollows 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:QueryRangeByLexWithNumericPrefixcovers the"0123"/"0123\xff"case from the issue, ascending and descending, through both the sync and async methods.QueryRangeByLexWithNumericPrefixAndExcludecovers"0123"/"0123xyz"withExclude.StartandExclude.Stop, in both orders.QueryRangeByLexWithDigitOnlyBoundschecks"2"/"10"in both orders against what Redis returns forZRANGEBYLEX key [10 [2andZREVRANGEBYLEX key [2 [10sent directly.QueryRangeByLexWithIntegerBoundsdoes the same with integer bounds,RedisValue min = 2; RedisValue max = 10;.QueryRangeByLexWithNumericPrefixAsBytesrepeats the"0123"case withbyte[]bounds holding the same UTF-8 bytes, and with a string bound against abyte[]bound.LengthByLexMatchesRangeWithDigitOnlyBoundschecks thatSortedSetLengthByValue(key, "2", "10"), sync and async, equals the number of membersSortedSetRangeByValuereturns, including withExclude.Start.RemoveRangeByLexDoesNotReorderBoundschecks thatSortedSetRemoveRangeByValue(key, "2", "10")removes nothing and leaves the set as it was.And in
RedisValueEquivalencyUnitTests, which needs no server,LexCompare_OrdersByRawBytesandLexCompare_DoesNotDependOnHowAValueIsStoredcheck thatLexCompareputs"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 andMemoryManager).The five range tests fail on
main, where each gets an empty result, and pass with the change. The length test fails onmainand on the first commit of this PR, whereZLEXCOUNTstill 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 (KeyPrefixedDatabaseTestsandKeyPrefixedTests, the only others that call these methods), andRedisValueEquivalencyUnitTestsgive 565 passed, 0 failed, 0 skipped. The library builds with no warnings on every target framework, and the public API files are unchanged.Checklist