Fix current position calculation in OldFormatDeserializer

Patch by Sam Tunnicliffe; reviewed by Jeff Jirsa for CASSANDRA-13525

This patch also adds a unit test for CASSANDRA-13236.

CHANGES.txt

@@ -1,4 +1,5 @@
 3.0.14
+ * ReverseIndexedReader may drop rows during 2.1 to 3.0 upgrade (CASSANDRA-13525)
  * Fix repair process violating start/end token limits for small ranges (CASSANDRA-13052)
  * Add storage port options to sstableloader (CASSANDRA-13518)
  * Properly handle quoted index names in cqlsh DESCRIBE output (CASSANDRA-12847)

src/java/org/apache/cassandra/db/UnfilteredDeserializer.java

@@ -256,6 +256,10 @@ public abstract class UnfilteredDeserializer
         // The position in the input after the last data consumption (readNext/skipNext).
         private long lastConsumedPosition;
 
+        // Tracks the size of the last LegacyAtom read from disk, because this needs to be accounted
+        // for when marking lastConsumedPosition after readNext/skipNext
+        private long bytesReadForNextAtom;
+
         private OldFormatDeserializer(CFMetaData metadata,
                                       DataInputPlus in,
                                       SerializationHelper helper,
@@ -272,7 +276,10 @@ public abstract class UnfilteredDeserializer
         {
             try
             {
-                return LegacyLayout.readLegacyAtom(metadata, in, readAllAsDynamic);
+                long pos = currentPosition();
+                LegacyLayout.LegacyAtom atom =  LegacyLayout.readLegacyAtom(metadata, in, readAllAsDynamic);
+                bytesReadForNextAtom = currentPosition() - pos;
+                return atom;
             }
             catch (IOException e)
             {
@@ -359,7 +366,7 @@ public abstract class UnfilteredDeserializer
                 throw new IllegalStateException();
             Unfiltered toReturn = next;
             next = null;
-            lastConsumedPosition = currentPosition();
+            lastConsumedPosition = currentPosition() - bytesReadForNextAtom();
             return toReturn;
         }
 
@@ -368,7 +375,7 @@ public abstract class UnfilteredDeserializer
             if (!hasNext())
                 throw new UnsupportedOperationException();
             next = null;
-            lastConsumedPosition = currentPosition();
+            lastConsumedPosition = currentPosition() - bytesReadForNextAtom();
         }
 
         public long bytesReadForUnconsumedData()
@@ -379,6 +386,21 @@ public abstract class UnfilteredDeserializer
             return currentPosition() - lastConsumedPosition;
         }
 
+        // Reading/skipping an Unfiltered consumes LegacyAtoms from the underlying legacy atom iterator
+        // e.g. hasNext() -> iterator.hasNext() -> iterator.readRow() -> atoms.next()
+        // The stop condition of the loop which groups legacy atoms into rows causes that AtomIterator
+        // to read in the first atom which doesn't belong in the row. So by that point, our position
+        // is actually past the end of the next Unfiltered. To compensate, we record the size of
+        // the last LegacyAtom read and subtract it from the current position when we calculate lastConsumedPosition.
+        // If we don't, then when reading an indexed block, we can over correct and may think that we've
+        // exhausted the block before we actually have.
+        private long bytesReadForNextAtom()
+        {
+            // If we've read anything at all then we will have recorded this in bytesReadForNextAtom,
+            // but being extra careful here just incase this method is called before any reads happen.
+            return iterator.atoms.next == null ? 0 : bytesReadForNextAtom;
+        }
+
         public void clearState()
         {
             next = null;

test/data/legacy-sstables/ka/legacy_tables/legacy_ka_indexed/legacy_tables-legacy_ka_indexed-ka-1-Digest.sha1

@@ -0,0 +1 @@
+76435450

test/data/legacy-sstables/ka/legacy_tables/legacy_ka_indexed/legacy_tables-legacy_ka_indexed-ka-1-TOC.txt

@@ -0,0 +1,8 @@
+Data.db
+Summary.db
+TOC.txt
+Statistics.db
+Digest.sha1
+Filter.db
+Index.db
+CompressionInfo.db

test/data/legacy-sstables/ka/legacy_tables/legacy_ka_indexed_static/legacy_tables-legacy_ka_indexed_static-ka-1-Digest.sha1

@@ -0,0 +1 @@
+3851004816

test/data/legacy-sstables/ka/legacy_tables/legacy_ka_indexed_static/legacy_tables-legacy_ka_indexed_static-ka-1-TOC.txt

@@ -0,0 +1,8 @@
+CompressionInfo.db
+Summary.db
+Data.db
+Index.db
+Statistics.db
+TOC.txt
+Digest.sha1
+Filter.db

test/unit/org/apache/cassandra/io/sstable/LegacySSTableTest.java

@@ -146,6 +146,45 @@ public class LegacySSTableTest
             verifyReads(legacyVersion);
         }
     }
+    @Test
+    public void testReverseIterationOfLegacyIndexedSSTable() throws Exception
+    {
+        // During upgrades from 2.1 to 3.0, reverse queries can drop rows before upgradesstables is completed
+        QueryProcessor.executeInternal("CREATE TABLE legacy_tables.legacy_ka_indexed (" +
+                                       "  p int," +
+                                       "  c int," +
+                                       "  v1 int," +
+                                       "  v2 int," +
+                                       "  PRIMARY KEY(p, c)" +
+                                       ")");
+        loadLegacyTable("legacy_%s_indexed%s", "ka", "");
+        UntypedResultSet rs = QueryProcessor.executeInternal("SELECT * " +
+                                                             "FROM legacy_tables.legacy_ka_indexed " +
+                                                             "WHERE p=1 " +
+                                                             "ORDER BY c DESC");
+        Assert.assertEquals(5000, rs.size());
+    }
+
+    @Test
+    public void testReadingLegacyIndexedSSTableWithStaticColumns() throws Exception
+    {
+        // During upgrades from 2.1 to 3.0, reading from tables with static columns errors before upgradesstables
+        // is completed
+        QueryProcessor.executeInternal("CREATE TABLE legacy_tables.legacy_ka_indexed_static (" +
+                                       "  p int," +
+                                       "  c int," +
+                                       "  v1 int," +
+                                       "  v2 int," +
+                                       "  s1 int static," +
+                                       "  s2 int static," +
+                                       "  PRIMARY KEY(p, c)" +
+                                       ")");
+        loadLegacyTable("legacy_%s_indexed_static%s", "ka", "");
+        UntypedResultSet rs = QueryProcessor.executeInternal("SELECT * " +
+                                                             "FROM legacy_tables.legacy_ka_indexed_static " +
+                                                             "WHERE p=1 ");
+        Assert.assertEquals(5000, rs.size());
+    }
 
     private void streamLegacyTables(String legacyVersion) throws Exception
     {