Issue mysql#48: index_read_map(HA_READ_PREFIX_LAST) does not work in …

…reverse CF

Summary:
Make ha_rocksdb::index_read_map() correctly handle find_flag values
HA_READ_PREFIX_LAST and HA_READ_PREFIX_LAST_OR_PREV.

Explanations how they should be handled are provided in
storage/rocksdb/rocksdb-range-access.txt

Test Plan:
mtr, mtr --gcov rocksdb_range, made sure that the new
code is all covered.

Reviewers: maykov, hermanlee4, jtolmer, yoshinorim

Reviewed By: yoshinorim

Differential Revision: https://reviews.facebook.net/D35415

storage/rocksdb/ha_rocksdb.cc

@@ -2691,8 +2691,6 @@ int ha_rocksdb::index_read_map(uchar *buf, const uchar *key,
     DBUG_RETURN(rc);
   }
 
-  uint n_used_parts= my_count_bits(keypart_map);
-
   uint packed_size;
 
   packed_size= kd->pack_index_tuple(table, pack_buffer,
@@ -2850,22 +2848,32 @@ int ha_rocksdb::index_read_map(uchar *buf, const uchar *key,
   case HA_READ_PREFIX_LAST_OR_PREV:
   {
     /*
-      Given a prefix of (VAL1,VAL2), get the last record that has
-      (kp1,kp2)=(VAL1,VAL2).  This cannot be translated directly to RocksDB
-      Iterator command.
-
-      We navigate to (VAL1,VAL2+1) and then step one record back.
+      Find the last record with the specified index prefix lookup_tuple.
+      - HA_READ_PREFIX_LAST requires that the record has the
+        prefix=lookup_tuple (if there are no such records,
+        HA_ERR_KEY_NOT_FOUND should be returned).
+      - HA_READ_PREFIX_LAST_OR_PREV has no such requirement. If there are no
+        records with prefix=lookup_tuple, we should return the last record
+        before that.
     */
-    if (!scan_it->Valid())
+    if (key_descr[active_index]->is_reverse_cf)
     {
-      /*
-        We're after the last value in the database. It could be we needed the
-        last one.
-      */
-      scan_it->SeekToLast();
+      if (using_full_key && scan_it->Valid())
+      {
+        rkey= scan_it->key();
+        if (kd->value_matches_prefix(rkey, slice))
+        {
+          scan_it->Next();
+        }
+      }
     }
     else
-      scan_it->Prev();
+    {
+      if (!scan_it->Valid())
+        scan_it->SeekToLast();
+      else
+        scan_it->Prev();
+    }
 
     if (scan_it->Valid())
     {
@@ -2875,20 +2883,26 @@ int ha_rocksdb::index_read_map(uchar *buf, const uchar *key,
       {
         /* The record we've got is not from this index */
         rc= HA_ERR_KEY_NOT_FOUND;
+        break;
       }
-      else
+
       if (find_flag == HA_READ_PREFIX_LAST)
       {
-        packed_size= kd->pack_index_tuple(table, pack_buffer,
-                                          sec_key_packed_tuple, key,
-                                          keypart_map);
         /*
-          Check if the record has the same search prefix.
+          We need to compare the key we've got (rkey) with the original
+          search prefix.
+          We don't have the original search prefix, because we've called
+          kd->successor() on it.  We'll need to prepare packed lookup tuple
+          again.
         */
-        if (kd->cmp_full_keys(rkey.data(), rkey.size(),
-                              (const char*)sec_key_packed_tuple, packed_size,
-                              n_used_parts))
-          rc= HA_ERR_END_OF_FILE;
+        uint size = kd->pack_index_tuple(table, pack_buffer,
+                                         sec_key_packed_tuple, key,
+                                         keypart_map);
+        rocksdb::Slice lookup_tuple((char*)sec_key_packed_tuple, size);
+        if (!kd->value_matches_prefix(rkey, lookup_tuple))
+        {
+          rc= HA_ERR_KEY_NOT_FOUND;
+        }
       }
     }
     else

storage/rocksdb/rocksdb-range-access.txt

@@ -279,3 +279,75 @@ RocksDB calls:
 
   if (it->Valid() && kd->covers_key(...))
     return record;
+
+== HA_READ_PREFIX_LAST, forward CF ==
+
+Find the last record with the specified index prefix lookup_tuple.
+
+Suppose, lookup_tuple='kv-bbb'
+
+  ( kv )-aaa-pk2
+  ( kv )-aaa-pk3
+# ( kv )-bbb
+  ( kv )-bbb-pk4
+  ( kv )-bbb-pk5
+  ( kv )-bbb-pk6
+  ( kv )-bbb-pk7 <--- Need to be here.
+# ( kv )-ccc
+  ( kv )-ccc-pk8 <-- Seek(Successor(kv-bbb)) will get us here. will need
+  ( kv )-ccc-pk9                               to call Prev().
+
+RocksDB calls:
+
+  Seek(Successor(kv-bbb));
+  if (!it->Valid())
+    it->SeekToLast();
+  else
+    it->Prev();
+
+  if (it->Valid() && kd->covers_key(...))
+  {
+    if (!cmp_full_keys(lookup_tuple))  // not needed in _OR_PREV
+    {
+      // the record's prefix matches lookup_tuple.
+      return record;
+    }
+  }
+
+== HA_READ_PREFIX_LAST, backward CF ==
+
+Suppose, lookup_tuple='kv-bbb'
+
+  ( kv )-ccc-pk9
+  ( kv )-ccc-pk8
+# ( kv )-ccc     <--  2. Seek(Successor(kv-bbb)) will point here
+                         and it will fall down to the next row.
+  ( kv )-bbb-pk7 <--- 1. Need to be here.
+  ( kv )-bbb-pk6
+  ( kv )-bbb-pk5
+  ( kv )-bbb-pk4
+# ( kv )-bbb
+  ( kv )-aaa-pk3
+  ( kv )-aaa-pk2
+
+
+RocksDB calls:
+
+  it->Seek(Successor(kv-bbb));
+
+  if (using_full_key && it->Valid() && !cmp_full_keys(Sucessor(lookup_key)))
+    it->Next();
+
+  if (it->Valid() && kd->covers_key(..))
+  {
+    if (!cmp_full_keys(...)) // not needed in _OR_PREV
+    {
+      // the record's prefix matches lookup_tuple.
+      return record;
+    }
+  }
+
+== HA_READ_PREFIX_LAST_OR_PREV, forward or backward CF ==
+
+This is just like HA_READ_PREFIX_LAST but we don't need to check that the key
+we've got is in the search prefix. (search for "not needed in _OR_PREV" above)