Duplicate rows after upgrading from 2.1.16 to 3.0.10/3.9

patch by Sylvain Lebresne; reviewed by Tyler Hobbs for CASSANDRA-13125

CHANGES.txt

@@ -1,4 +1,5 @@
 3.0.11
+ * Duplicate rows after upgrading from 2.1.16 to 3.0.10/3.9 (CASSANDRA-13125)
  * Fix UPDATE queries with empty IN restrictions (CASSANDRA-13152)
  * Abort or retry on failed hints delivery (CASSANDRA-13124)
  * Fix handling of partition with partition-level deletion plus

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

@@ -1803,17 +1803,42 @@ public abstract class LegacyLayout
             if (a.isStatic != b.isStatic)
                 return a.isStatic ? -1 : 1;
 
-            int result = this.clusteringComparator.compare(a.bound, b.bound);
-            if (result != 0)
-                return result;
+            // We have to be careful with bound comparison because of collections. Namely, if the 2 bounds represent the
+            // same prefix, then we should take the collectionName into account before taking the bounds kind
+            // (ClusteringPrefix.Kind). This means we can't really call ClusteringComparator.compare() directly.
+            // For instance, if
+            //    a is (bound=INCL_START_BOUND('x'), collectionName='d')
+            //    b is (bound=INCL_END_BOUND('x'),   collectionName='c')
+            // Ten b < a since the element 'c' of collection 'x' comes before element 'd', but calling
+            // clusteringComparator.compare(a.bound, b.bound) returns -1.
+            // See CASSANDRA-13125 for details.
+            int sa = a.bound.size();
+            int sb = b.bound.size();
+            for (int i = 0; i < Math.min(sa, sb); i++)
+            {
+                int cmp = clusteringComparator.compareComponent(i, a.bound.get(i), b.bound.get(i));
+                if (cmp != 0)
+                    return cmp;
+            }
 
-            // If both have equal "bound" but one is a collection tombstone and not the other, then the other comes before as it points to the beginning of the row.
-            if (a.collectionName == null)
-                return b.collectionName == null ? 0 : 1;
-            if (b.collectionName == null)
-                return -1;
+            if (sa != sb)
+                return sa < sb ? a.bound.kind().comparedToClustering : -b.bound.kind().comparedToClustering;
 
-            return UTF8Type.instance.compare(a.collectionName.name.bytes, b.collectionName.name.bytes);
+            // Both bound represent the same prefix, compare the collection names
+            // If one has a collection name and the other doesn't, the other comes before as it points to the beginning of the row.
+            if ((a.collectionName == null) != (b.collectionName == null))
+                return a.collectionName == null ? -1 : 1;
+
+            // If they both have a collection, compare that first
+            if (a.collectionName != null)
+            {
+                int cmp = UTF8Type.instance.compare(a.collectionName.name.bytes, b.collectionName.name.bytes);
+                if (cmp != 0)
+                    return cmp;
+            }
+
+            // Lastly, if everything so far is equal, compare their clustering kind
+            return ClusteringPrefix.Kind.compare(a.bound.kind(), b.bound.kind());
         }
     }
 
@@ -1830,8 +1855,8 @@ public abstract class LegacyLayout
 
         // Note: we don't want to use a List for the markedAts and delTimes to avoid boxing. We could
         // use a List for starts and ends, but having arrays everywhere is almost simpler.
-        private LegacyBound[] starts;
-        private LegacyBound[] ends;
+        LegacyBound[] starts;
+        LegacyBound[] ends;
         private long[] markedAts;
         private int[] delTimes;
 
@@ -1853,6 +1878,20 @@ public abstract class LegacyLayout
             this(comparator, new LegacyBound[capacity], new LegacyBound[capacity], new long[capacity], new int[capacity], 0);
         }
 
+        @Override
+        public String toString()
+        {
+            StringBuilder sb = new StringBuilder();
+            sb.append('[');
+            for (int i = 0; i < size; i++)
+            {
+                if (i > 0)
+                    sb.append(',');
+                sb.append('(').append(starts[i]).append(", ").append(ends[i]).append(')');
+            }
+            return sb.append(']').toString();
+        }
+
         public boolean isEmpty()
         {
             return size == 0;
@@ -2107,6 +2146,7 @@ public abstract class LegacyLayout
             // If we got there, then just insert the remainder at the end
             addInternal(i, start, end, markedAt, delTime);
         }
+
         private int capacity()
         {
             return starts.length;

test/unit/org/apache/cassandra/db/LegacyLayoutTest.java

@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db;
+
+import java.nio.ByteBuffer;
+
+import org.junit.Test;
+
+import org.apache.cassandra.config.CFMetaData;
+import org.apache.cassandra.config.ColumnDefinition;
+import org.apache.cassandra.cql3.ColumnIdentifier;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.SetType;
+import org.apache.cassandra.db.partitions.PartitionUpdate;
+import org.apache.cassandra.db.rows.BTreeRow;
+import org.apache.cassandra.db.rows.Row;
+import org.apache.cassandra.utils.ByteBufferUtil;
+
+import static org.junit.Assert.*;
+
+public class LegacyLayoutTest
+{
+    @Test
+    public void testFromUnfilteredRowIterator() throws Throwable
+    {
+        CFMetaData table = CFMetaData.Builder.create("ks", "table")
+                                             .addPartitionKey("k", Int32Type.instance)
+                                             .addRegularColumn("a", SetType.getInstance(Int32Type.instance, true))
+                                             .addRegularColumn("b", SetType.getInstance(Int32Type.instance, true))
+                                             .build();
+
+        ColumnDefinition a = table.getColumnDefinition(new ColumnIdentifier("a", false));
+        ColumnDefinition b = table.getColumnDefinition(new ColumnIdentifier("b", false));
+
+        Row.Builder builder = BTreeRow.unsortedBuilder(0);
+        builder.newRow(Clustering.EMPTY);
+        builder.addComplexDeletion(a, new DeletionTime(1L, 1));
+        builder.addComplexDeletion(b, new DeletionTime(1L, 1));
+        Row row = builder.build();
+
+        ByteBuffer key = ByteBufferUtil.bytes(1);
+        PartitionUpdate upd = PartitionUpdate.singleRowUpdate(table, key, row);
+
+        LegacyLayout.LegacyUnfilteredPartition p = LegacyLayout.fromUnfilteredRowIterator(null, upd.unfilteredIterator());
+        assertEquals(DeletionTime.LIVE, p.partitionDeletion);
+        assertEquals(0, p.cells.size());
+
+        LegacyLayout.LegacyRangeTombstoneList l = p.rangeTombstones;
+        assertEquals("a", l.starts[0].collectionName.name.toString());
+        assertEquals("a", l.ends[0].collectionName.name.toString());
+
+        assertEquals("b", l.starts[1].collectionName.name.toString());
+        assertEquals("b", l.ends[1].collectionName.name.toString());
+    }
+}