RavenDB-22964 Fix BackwardIterator

src/Corax/Querying/Matches/TermProviders/TermProvider.NumericRange.cs

@@ -32,7 +32,7 @@ public struct TermNumericRangeProvider<TLookupIterator, TLow, THigh, TVal> : 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;

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<TLookupKey>(out TLookupKey key, out long value, out bool hasPreviousValue);
+
+        /// <summary>
+        /// 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
+        /// </summary>
+        /// <param name="key">Lookup key</param>
+        /// <typeparam name="TLookupKey">Type of lookup key used for seek.</typeparam>
         void Seek<TLookupKey>(TLookupKey key);
     }
 
@@ -161,6 +175,11 @@ public int Fill(Span<long> results, long lastId = long.MaxValue, bool includeMax
                 }
             }
 
+            /// <summary>
+            /// Returns currently pointed value and moves to the next element in the tree.
+            /// </summary>
+            /// <param name="value">Current (before MoveNext) value.</param>
+            /// <returns>Indicates if current exists.</returns>
             public bool MoveNext(out long value)
             {
                 if (_cursor._pos < 0)
@@ -276,8 +295,11 @@ public void Seek<T>(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<long> 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<T>(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));

src/Voron/Data/Lookups/Lookup.cs

@@ -190,7 +190,15 @@ private struct IteratorCursorState
     public struct CursorState
     {
         public Page Page;
+
+        /// <summary>
+        /// 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
+        /// </summary>
         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());
     }
 

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<Int64LookupKey>("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<Int64LookupKey>("test");
+
+            // 5 4 2 1
+            var it = tree.Iterate<Lookup<Int64LookupKey>.BackwardIterator>();
+
+            bool moveNext;
+            long value;
+
+            it.Reset();
+            it.Seek<Int64LookupKey>(4);
+            moveNext = it.MoveNext(out value);
+            Assert.True(moveNext);
+            Assert.Equal(4, value);
+
+            it.Reset();
+            it.Seek<Int64LookupKey>(3);
+            moveNext = it.MoveNext(out value);
+            Assert.True(moveNext);
+            Assert.Equal(2, value);
+
+            it.Reset();
+            it.Seek<Int64LookupKey>(0);
+            moveNext = it.MoveNext(out value);
+            Assert.False(moveNext);
+
+            it.Reset();
+            it.Seek<Int64LookupKey>(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<Int64LookupKey>("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<Int64LookupKey>("test");
+
+            // 4998 4996 ... 2 0
+            var it = tree.Iterate<Lookup<Int64LookupKey>.BackwardIterator>();
+
+            var numberOfPages = tree.AllPages().Count;
+
+            Assert.Equal(4, numberOfPages);
+
+            bool moveNext;
+            long value;
+
+            it.Reset();
+            it.Seek<Int64LookupKey>(2);
+            moveNext = it.MoveNext(out value);
+            Assert.True(moveNext);
+            Assert.Equal(2, value);
+
+            it.Reset();
+            it.Seek<Int64LookupKey>(1900);
+            moveNext = it.MoveNext(out value);
+            Assert.True(moveNext);
+            Assert.Equal(1900, value);
+
+            it.Reset();
+            it.Seek<Int64LookupKey>(3);
+            moveNext = it.MoveNext(out value);
+            Assert.True(moveNext);
+            Assert.Equal(2, value);
+
+            it.Reset();
+            it.Seek<Int64LookupKey>(1899);
+            moveNext = it.MoveNext(out value);
+            Assert.True(moveNext);
+            Assert.Equal(1898, value);
+        }
+    }
+
     [Fact]
     public void CanProperlyResetIterator()
     {