diff --git a/src/Corax/Querying/Matches/TermProviders/TermProvider.NumericRange.cs b/src/Corax/Querying/Matches/TermProviders/TermProvider.NumericRange.cs index 7497799fb9a1..30acb6b4dba8 100644 --- a/src/Corax/Querying/Matches/TermProviders/TermProvider.NumericRange.cs +++ b/src/Corax/Querying/Matches/TermProviders/TermProvider.NumericRange.cs @@ -32,7 +32,7 @@ public struct TermNumericRangeProvider : ITe private readonly FieldMetadata _field; private TVal _low, _high; private TLookupIterator _iterator; - private readonly bool _skipRangeCheck; + private bool _skipRangeCheck; private long _lastTermId = -1; private bool _includeLastTerm = true; private bool _isEmpty; @@ -59,56 +59,74 @@ false when typeof(TLow) == typeof(Range.Inclusive) && typeof(TVal) == typeof(Dou PrepareKeys(); Reset(); } - + private void PrepareKeys() { + // We want to rewrite the range, so it starts and ends with elements that are presented in our data source TVal startKey = _iterator.IsForward ? _low : _high; TVal finalKey = _iterator.IsForward ? _high : _low; + // Seek for startKey in the iterator _iterator.Seek(startKey); - if (_iterator.MoveNext(out TVal key, out _, out _) == false) + + // We get either + // currentKey equal to startKey + // currentKey equal to element succeeding startKey + // MoveNext returns false + if (_iterator.MoveNext(out TVal currentKey, out _, out _) == false) { + // When MoveNext returns false it means we jumped over all values stored inside a lookup tree _isEmpty = true; return; } - var skipFirst = _iterator.IsForward switch - { - true when typeof(TLow) == typeof(Range.Exclusive) && key.IsEqual(_low) => true, - false when typeof(THigh) == typeof(Range.Exclusive) && _high.CompareTo(key) <= 0 => true, - false when typeof(THigh) == typeof(Range.Inclusive) && _high.CompareTo(key) < 0 => true, - _ => false - }; - + // The first element may be equal to the startKey. For exclusive range start we want to skip it + var skipFirst = (_iterator.IsForward ? typeof(TLow) : typeof(THigh)) == typeof(Range.Exclusive) + && startKey.IsEqual(currentKey); + + // If we're supposed to skip first element, we rewrite the range to start from element succeeding startKey if (skipFirst) { - if (_iterator.MoveNext(out key, out _, out _) == false) + if (_iterator.MoveNext(out currentKey, out _, out _) == false) { + // We moved past queried range, nothing matches the query _isEmpty = true; return; } - - if (_iterator.IsForward) - _low = key; - else - _high = key; } + // Update the range accordingly to the iterator option + if (_iterator.IsForward) + _low = currentKey; + else + _high = currentKey; + if (_skipRangeCheck) - return; // to the end + return; - //Now seek to the last key + // Seek for finalKey in the iterator _iterator.Seek(finalKey); - if (_iterator.MoveNext(out key, out _lastTermId, out var hasPreviousValue) == false) + + // We get either + // currentKey equal to finalKey + // currentKey equal to element succeeding finalKey + // MoveNext returns false + if (_iterator.MoveNext(out currentKey, out _lastTermId, out _) == false) { - _includeLastTerm = false; + // We jumped over all data stored in the tree. Other side of the range is unbound + _skipRangeCheck = true; return; } - + _includeLastTerm = true; + + // 1 - finalKey > currentKey + // 0 - finalKey == currentKey + // -1 - finalKey < currentKey (not possible) + var cmp = finalKey.CompareTo(currentKey); + if (_iterator.IsForward) { - var cmp = _high.CompareTo(key); if (typeof(THigh) == typeof(Range.Exclusive) && cmp <= 0 || typeof(THigh) == typeof(Range.Inclusive) && cmp < 0) { @@ -117,7 +135,6 @@ false when typeof(THigh) == typeof(Range.Inclusive) && _high.CompareTo(key) < 0 } else { - var cmp = _low.CompareTo(key); if (typeof(TLow) == typeof(Range.Exclusive) && cmp >= 0 || typeof(TLow) == typeof(Range.Inclusive) && cmp > 0) _includeLastTerm = false; diff --git a/src/Voron/Data/Lookups/Lookup.Iterator.cs b/src/Voron/Data/Lookups/Lookup.Iterator.cs index 10544668966d..db6f5d0a0698 100644 --- a/src/Voron/Data/Lookups/Lookup.Iterator.cs +++ b/src/Voron/Data/Lookups/Lookup.Iterator.cs @@ -12,6 +12,20 @@ public interface ILookupIterator bool Skip(long count); bool MoveNext(out long value); bool MoveNext(out TLookupKey key, out long value, out bool hasPreviousValue); + + /// + /// Moves iterator to the element equal to key parameter + /// + /// If equal element doesn't exist, moves iterator to element that succeeds it: + /// Forward - the smallest element that's bigger than key parameter + /// Backward - the biggest element that's smaller than key parameter + /// + /// If succeeding element doesn't exist, moves iterator (LastSearchPosition) to: + /// ForwardIterator - position n, where n is the number of elements (effectively moving one position after actual data) + /// BackwardIterator - position -1 + /// + /// Lookup key + /// Type of lookup key used for seek. void Seek(TLookupKey key); } @@ -161,6 +175,11 @@ public int Fill(Span results, long lastId = long.MaxValue, bool includeMax } } + /// + /// Returns currently pointed value and moves to the next element in the tree. + /// + /// Current (before MoveNext) value. + /// Indicates if current exists. public bool MoveNext(out long value) { if (_cursor._pos < 0) @@ -276,8 +295,11 @@ public void Seek(T key) _tree.FindPageFor(ref lookupKey, ref _cursor); ref var state = ref _cursor._stk[_cursor._pos]; - if (state.LastSearchPosition < 0) - state.LastSearchPosition = Math.Min(~state.LastSearchPosition, state.Header->NumberOfEntries - 1); + + if (state.LastSearchPosition >= 0) + return; + + state.LastSearchPosition = ~state.LastSearchPosition - 1; } @@ -343,7 +365,7 @@ public int Fill(Span results, long lastTermId = long.MaxValue, bool includ if (state.LastSearchPosition >= 0) { int read = 0; - while(read < results.Length && state.LastSearchPosition >= 0) + while (read < results.Length && state.LastSearchPosition >= 0) { int curPos = state.LastSearchPosition--; results[read++] = GetValue(ref state, curPos); @@ -373,7 +395,9 @@ public bool MoveNext(out long value) value = default; return false; } + ref var state = ref _cursor._stk[_cursor._pos]; + while (true) { Debug.Assert(state.Header->PageFlags.HasFlag(LookupPageFlags.Leaf)); @@ -407,6 +431,7 @@ public bool MoveNext(out T key, out long value, out bool hasPreviousValue) return false; } ref var state = ref _cursor._stk[_cursor._pos]; + while (true) { Debug.Assert(state.Header->PageFlags.HasFlag(LookupPageFlags.Leaf)); diff --git a/src/Voron/Data/Lookups/Lookup.cs b/src/Voron/Data/Lookups/Lookup.cs index 8ed2f38cbaa7..012e38dd9d5a 100644 --- a/src/Voron/Data/Lookups/Lookup.cs +++ b/src/Voron/Data/Lookups/Lookup.cs @@ -190,7 +190,15 @@ private struct IteratorCursorState public struct CursorState { public Page Page; + + /// + /// Values: + /// 0: searched value was found + /// 1: searched value was bigger than last compared value in the page + /// -1: searched value was smaller than last compared value in the page + /// public int LastMatch; + public int LastSearchPosition; public LookupPageHeader* Header => (LookupPageHeader*)Page.Pointer; @@ -1304,6 +1312,9 @@ private void SearchInCurrentPage(ref TLookupKey key, ref CursorState state, int NotFound: state.LastMatch = match > 0 ? 1 : -1; + // Negation gives information we didn't find the key in the page + // In this case, if last compared value is smaller than seeked key, we may suspect it will be the next element of the tree. + // It points to the smallest element that is bigger than seeked key. state.LastSearchPosition = ~(bot + (match > 0).ToInt32()); } diff --git a/test/FastTests/Corax/CompactTreeTests.cs b/test/FastTests/Corax/CompactTreeTests.cs index a60e81feb238..a670a83bdb6b 100644 --- a/test/FastTests/Corax/CompactTreeTests.cs +++ b/test/FastTests/Corax/CompactTreeTests.cs @@ -122,6 +122,109 @@ public void CanHandlePageSplitsWithCompression() } } + [Fact] + public void TestSeekForBackwardIterator() + { + using (var wtx = Env.WriteTransaction()) + { + var tree = wtx.LookupFor("test"); + tree.Add(new Int64LookupKey(1), 1); + tree.Add(new Int64LookupKey(2), 2); + tree.Add(new Int64LookupKey(4), 4); + tree.Add(new Int64LookupKey(5), 5); + wtx.Commit(); + } + + using (var rtx = Env.ReadTransaction()) + { + var tree = rtx.LookupFor("test"); + + // 5 4 2 1 + var it = tree.Iterate.BackwardIterator>(); + + bool moveNext; + long value; + + it.Reset(); + it.Seek(4); + moveNext = it.MoveNext(out value); + Assert.True(moveNext); + Assert.Equal(4, value); + + it.Reset(); + it.Seek(3); + moveNext = it.MoveNext(out value); + Assert.True(moveNext); + Assert.Equal(2, value); + + it.Reset(); + it.Seek(0); + moveNext = it.MoveNext(out value); + Assert.False(moveNext); + + it.Reset(); + it.Seek(6); + moveNext = it.MoveNext(out value); + Assert.True(moveNext); + Assert.Equal(5, value); + } + } + + [Fact] + public void TestSeekForBackwardIteratorUsingMultiplePages() + { + using (var wtx = Env.WriteTransaction()) + { + var tree = wtx.LookupFor("test"); + + for (int i = 0; i < 5_000; i += 2) + { + tree.Add(new Int64LookupKey(i), i); + } + + wtx.Commit(); + } + + using (var rtx = Env.ReadTransaction()) + { + var tree = rtx.LookupFor("test"); + + // 4998 4996 ... 2 0 + var it = tree.Iterate.BackwardIterator>(); + + var numberOfPages = tree.AllPages().Count; + + Assert.Equal(4, numberOfPages); + + bool moveNext; + long value; + + it.Reset(); + it.Seek(2); + moveNext = it.MoveNext(out value); + Assert.True(moveNext); + Assert.Equal(2, value); + + it.Reset(); + it.Seek(1900); + moveNext = it.MoveNext(out value); + Assert.True(moveNext); + Assert.Equal(1900, value); + + it.Reset(); + it.Seek(3); + moveNext = it.MoveNext(out value); + Assert.True(moveNext); + Assert.Equal(2, value); + + it.Reset(); + it.Seek(1899); + moveNext = it.MoveNext(out value); + Assert.True(moveNext); + Assert.Equal(1898, value); + } + } + [Fact] public void CanProperlyResetIterator() { diff --git a/test/SlowTests/Issues/RavenDB-22964.cs b/test/SlowTests/Issues/RavenDB-22964.cs new file mode 100644 index 000000000000..213e8d941a87 --- /dev/null +++ b/test/SlowTests/Issues/RavenDB-22964.cs @@ -0,0 +1,171 @@ +using System.Linq; +using FastTests; +using Tests.Infrastructure; +using Xunit; +using Xunit.Abstractions; + +namespace SlowTests.Issues; + +public class RavenDB_22964 : RavenTestBase +{ + public RavenDB_22964(ITestOutputHelper output) : base(output) + { + } + + [RavenTheory(RavenTestCategory.Corax | RavenTestCategory.Querying)] + [RavenData(SearchEngineMode = RavenSearchEngineMode.Corax, DatabaseMode = RavenDatabaseMode.All)] + public void TestOrderByDescending(Options options) + { + using (var store = GetDocumentStore(options)) + { + using (var session = store.OpenSession()) + { + var t1 = new TestObject() { Value = 1 }; + var t2 = new TestObject() { Value = 3 }; + var t3 = new TestObject() { Value = 4 }; + var t4 = new TestObject() { Value = 5 }; + + session.Store(t1); + session.Store(t2); + session.Store(t3); + session.Store(t4); + session.SaveChanges(); + + long min = 2; + long max = 4; + + var r0 = session.Query() + .Where(x => x.Value >= min && x.Value < max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(1, r0.Count); + Assert.Equal(3, r0[0].Value); + + var r1 = session.Query() + .Where(x => x.Value > min && x.Value < max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(1, r1.Count); + Assert.Equal(3, r1[0].Value); + + var r2 = session.Query() + .Where(x => x.Value > min && x.Value <= max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(2, r2.Count); + Assert.Equal(4, r2[0].Value); + Assert.Equal(3, r2[1].Value); + + var r3 = session.Query() + .Where(x => x.Value >= min && x.Value <= max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(2, r3.Count); + Assert.Equal(4, r3[0].Value); + Assert.Equal(3, r3[1].Value); + + min = 0; + + var r4 = session.Query() + .Where(x => x.Value > min && x.Value < max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(2, r4.Count); + Assert.Equal(3, r4[0].Value); + Assert.Equal(1, r4[1].Value); + + max = 7; + + var r5 = session.Query() + .Where(x => x.Value > min && x.Value < max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(4, r5.Count); + Assert.Equal(5, r5[0].Value); + Assert.Equal(4, r5[1].Value); + Assert.Equal(3, r5[2].Value); + Assert.Equal(1, r5[3].Value); + + min = 10; + max = 15; + + var r6 = session.Query() + .Where(x => x.Value > min && x.Value < max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(0, r6.Count); + + min = -5; + max = -2; + + var r7 = session.Query() + .Where(x => x.Value > min && x.Value < max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(0, r7.Count); + } + } + } + + [RavenTheory(RavenTestCategory.Corax | RavenTestCategory.Querying)] + [RavenData(SearchEngineMode = RavenSearchEngineMode.Corax, DatabaseMode = RavenDatabaseMode.All)] + public void TestRangeBetweenValues(Options options) + { + using (var store = GetDocumentStore(options)) + { + using (var session = store.OpenSession()) + { + var t1 = new TestObject() { Value = 2 }; + var t2 = new TestObject() { Value = 24 }; + + session.Store(t1); + session.Store(t2); + session.SaveChanges(); + + const long min = 6; + const long max = 12; + + var r1 = session.Query() + .Where(x => x.Value > min && x.Value < max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(0, r1.Count); + + var r2 = session.Query() + .Where(x => x.Value >= min && x.Value < max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(0, r2.Count); + + var r3 = session.Query() + .Where(x => x.Value > min && x.Value <= max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(0, r3.Count); + + var r4 = session.Query() + .Where(x => x.Value >= min && x.Value <= max) + .OrderByDescending(x => x.Value) + .ToList(); + + Assert.Equal(0, r4.Count); + } + } + } + + private class TestObject + { + public required long Value { get; set; } + } +}