You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by sl...@apache.org on 2020/05/27 15:19:31 UTC
[cassandra] 02/02: Fix LegacyLayout handling of non-selected
collection tombstones
This is an automated email from the ASF dual-hosted git repository.
slebresne pushed a commit to branch cassandra-3.0
in repository https://gitbox.apache.org/repos/asf/cassandra.git
commit c8a2834606d683ba9945e9cc11bdb4207ce269d1
Author: Sylvain Lebresne <le...@gmail.com>
AuthorDate: Wed May 13 11:44:08 2020 +0200
Fix LegacyLayout handling of non-selected collection tombstones
If a collection tombstone is not included by a query, it can be ignored,
but it currently made `LegacyLayout.CellGrouper#addCollectionTombstone`
return `false`, which made it stop the current row, which is incorrect
(this can potentially lead to a duplicate row). This patch changes it
to return `true`.
Patch by Sylvain Lebresne; reviewed by Marcus Eriksson & Aleksey
Yeschenko for CASSANDRA-15805
---
src/java/org/apache/cassandra/db/LegacyLayout.java | 6 +-
.../org/apache/cassandra/db/LegacyLayoutTest.java | 102 +++++++++++++++++----
2 files changed, 88 insertions(+), 20 deletions(-)
diff --git a/src/java/org/apache/cassandra/db/LegacyLayout.java b/src/java/org/apache/cassandra/db/LegacyLayout.java
index 39dd54a..8492de5 100644
--- a/src/java/org/apache/cassandra/db/LegacyLayout.java
+++ b/src/java/org/apache/cassandra/db/LegacyLayout.java
@@ -1537,8 +1537,12 @@ public abstract class LegacyLayout
private boolean addCollectionTombstone(LegacyRangeTombstone tombstone)
{
+ // If the collection tombstone is not included in the query (which technically would only apply to thrift
+ // queries since CQL one "fetch" everything), we can skip it (so return), but we're problably still within
+ // the current row so we return `true`. Technically, it is possible that tombstone belongs to another row
+ // that the row currently grouped, but as we ignore it, returning `true` is ok in that case too.
if (!helper.includes(tombstone.start.collectionName))
- return false; // see CASSANDRA-13109
+ return true; // see CASSANDRA-13109
// The helper needs to be informed about the current complex column identifier before
// it can perform the comparison between the recorded drop time and the RT deletion time.
diff --git a/test/unit/org/apache/cassandra/db/LegacyLayoutTest.java b/test/unit/org/apache/cassandra/db/LegacyLayoutTest.java
index 0bb2459..f0d2a02 100644
--- a/test/unit/org/apache/cassandra/db/LegacyLayoutTest.java
+++ b/test/unit/org/apache/cassandra/db/LegacyLayoutTest.java
@@ -24,18 +24,19 @@ import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
+import org.apache.cassandra.db.LegacyLayout.CellGrouper;
+import org.apache.cassandra.db.LegacyLayout.LegacyBound;
+import org.apache.cassandra.db.LegacyLayout.LegacyCell;
+import org.apache.cassandra.db.LegacyLayout.LegacyRangeTombstone;
import org.apache.cassandra.db.filter.ColumnFilter;
import org.apache.cassandra.db.marshal.MapType;
import org.apache.cassandra.db.marshal.UTF8Type;
-import org.apache.cassandra.db.partitions.ImmutableBTreePartition;
import org.apache.cassandra.db.rows.BufferCell;
import org.apache.cassandra.db.rows.Cell;
import org.apache.cassandra.db.rows.RowIterator;
-import org.apache.cassandra.db.rows.Rows;
import org.apache.cassandra.db.rows.SerializationHelper;
import org.apache.cassandra.db.rows.UnfilteredRowIterator;
import org.apache.cassandra.db.rows.UnfilteredRowIteratorSerializer;
-import org.apache.cassandra.db.rows.UnfilteredRowIterators;
import org.apache.cassandra.db.transform.FilteredRows;
import org.apache.cassandra.exceptions.ConfigurationException;
import org.apache.cassandra.io.util.DataInputBuffer;
@@ -62,10 +63,10 @@ import org.apache.cassandra.db.rows.BTreeRow;
import org.apache.cassandra.db.rows.Row;
import org.apache.cassandra.dht.Murmur3Partitioner;
import org.apache.cassandra.schema.KeyspaceParams;
-import org.apache.cassandra.utils.ByteBufferUtil;
import org.apache.cassandra.utils.Hex;
import static org.apache.cassandra.net.MessagingService.VERSION_21;
+import static org.apache.cassandra.utils.ByteBufferUtil.bytes;
import static org.junit.Assert.*;
public class LegacyLayoutTest
@@ -98,7 +99,7 @@ public class LegacyLayoutTest
builder.addComplexDeletion(b, new DeletionTime(1L, 1));
Row row = builder.build();
- ByteBuffer key = ByteBufferUtil.bytes(1);
+ ByteBuffer key = bytes(1);
PartitionUpdate upd = PartitionUpdate.singleRowUpdate(table, key, row);
LegacyLayout.LegacyUnfilteredPartition p = LegacyLayout.fromUnfilteredRowIterator(null, upd.unfilteredIterator());
@@ -216,7 +217,7 @@ public class LegacyLayoutTest
builder.addCell(new BufferCell(v, 1L, Cell.NO_TTL, Cell.NO_DELETION_TIME, Int32Serializer.instance.serialize(1), null));
Row row = builder.build();
- DecoratedKey pk = table.decorateKey(ByteBufferUtil.bytes(1));
+ DecoratedKey pk = table.decorateKey(bytes(1));
PartitionUpdate upd = PartitionUpdate.singleRowUpdate(table, pk, row, staticRow);
try (RowIterator before = FilteredRows.filter(upd.unfilteredIterator(), FBUtilities.nowInSeconds());
@@ -243,7 +244,7 @@ public class LegacyLayoutTest
builder.addComplexDeletion(bug, new DeletionTime(1L, 1));
Row row = builder.build();
- DecoratedKey pk = table.decorateKey(ByteBufferUtil.bytes(1));
+ DecoratedKey pk = table.decorateKey(bytes(1));
PartitionUpdate upd = PartitionUpdate.singleRowUpdate(table, pk, row);
UnfilteredRowIterator afterRoundTripVia32 = roundTripVia21(upd.unfilteredIterator());
@@ -277,7 +278,7 @@ public class LegacyLayoutTest
builder.addComplexDeletion(bug, new DeletionTime(1L, 1));
Row row = builder.build();
- DecoratedKey pk = table.decorateKey(ByteBufferUtil.bytes(1));
+ DecoratedKey pk = table.decorateKey(bytes(1));
PartitionUpdate upd = PartitionUpdate.singleRowUpdate(table, pk, row);
// we need to perform the round trip in two parts here, with a column drop inbetween
@@ -385,26 +386,89 @@ public class LegacyLayoutTest
SerializationHelper helper = new SerializationHelper(cfm, MessagingService.VERSION_22, SerializationHelper.Flag.LOCAL, ColumnFilter.all(cfm));
LegacyLayout.CellGrouper cg = new LegacyLayout.CellGrouper(cfm, helper);
- Slice.Bound startBound = Slice.Bound.create(ClusteringPrefix.Kind.INCL_START_BOUND, new ByteBuffer[] {ByteBufferUtil.bytes(2)});
- Slice.Bound endBound = Slice.Bound.create(ClusteringPrefix.Kind.EXCL_END_BOUND, new ByteBuffer[] {ByteBufferUtil.bytes(2)});
- LegacyLayout.LegacyBound start = new LegacyLayout.LegacyBound(startBound, false, cfm.getColumnDefinition(ByteBufferUtil.bytes("v")));
- LegacyLayout.LegacyBound end = new LegacyLayout.LegacyBound(endBound, false, cfm.getColumnDefinition(ByteBufferUtil.bytes("v")));
+ Slice.Bound startBound = Slice.Bound.create(ClusteringPrefix.Kind.INCL_START_BOUND, new ByteBuffer[] {bytes(2)});
+ Slice.Bound endBound = Slice.Bound.create(ClusteringPrefix.Kind.EXCL_END_BOUND, new ByteBuffer[] {bytes(2)});
+ LegacyLayout.LegacyBound start = new LegacyLayout.LegacyBound(startBound, false, cfm.getColumnDefinition(bytes("v")));
+ LegacyLayout.LegacyBound end = new LegacyLayout.LegacyBound(endBound, false, cfm.getColumnDefinition(bytes("v")));
LegacyLayout.LegacyRangeTombstone lrt = new LegacyLayout.LegacyRangeTombstone(start, end, new DeletionTime(2, 1588598040));
assertTrue(cg.addAtom(lrt));
// add a real cell
LegacyLayout.LegacyCell cell = new LegacyLayout.LegacyCell(LegacyLayout.LegacyCell.Kind.REGULAR,
- new LegacyLayout.LegacyCellName(new Clustering(ByteBufferUtil.bytes(2)),
- cfm.getColumnDefinition(ByteBufferUtil.bytes("v")),
- ByteBufferUtil.bytes("g")),
- ByteBufferUtil.bytes("v"), 3, Integer.MAX_VALUE, 0);
+ new LegacyLayout.LegacyCellName(new Clustering(bytes(2)),
+ cfm.getColumnDefinition(bytes("v")),
+ bytes("g")),
+ bytes("v"), 3, Integer.MAX_VALUE, 0);
assertTrue(cg.addAtom(cell));
// add legacy range tombstone where collection name is null for the end bound (this gets translated to a row tombstone)
- startBound = Slice.Bound.create(ClusteringPrefix.Kind.EXCL_START_BOUND, new ByteBuffer[] {ByteBufferUtil.bytes(2)});
- endBound = Slice.Bound.create(ClusteringPrefix.Kind.EXCL_END_BOUND, new ByteBuffer[] {ByteBufferUtil.bytes(2)});
- start = new LegacyLayout.LegacyBound(startBound, false, cfm.getColumnDefinition(ByteBufferUtil.bytes("v")));
+ startBound = Slice.Bound.create(ClusteringPrefix.Kind.EXCL_START_BOUND, new ByteBuffer[] {bytes(2)});
+ endBound = Slice.Bound.create(ClusteringPrefix.Kind.EXCL_END_BOUND, new ByteBuffer[] {bytes(2)});
+ start = new LegacyLayout.LegacyBound(startBound, false, cfm.getColumnDefinition(bytes("v")));
end = new LegacyLayout.LegacyBound(endBound, false, null);
assertTrue(cg.addAtom(new LegacyLayout.LegacyRangeTombstone(start, end, new DeletionTime(1, 1588598040))));
}
+
+ private static LegacyCell cell(Clustering clustering, ColumnDefinition column, ByteBuffer value, long timestamp)
+ {
+ return new LegacyCell(LegacyCell.Kind.REGULAR,
+ new LegacyLayout.LegacyCellName(clustering, column, null),
+ value,
+ timestamp,
+ Cell.NO_DELETION_TIME,
+ Cell.NO_TTL);
+ }
+
+ /**
+ * This tests that when {@link CellGrouper} gets a collection tombstone for
+ * a non-fetched collection, then that tombstone does not incorrectly stop the grouping of the current row, as
+ * was done before CASSANDRA-15805.
+ *
+ * <p>Please note that this rely on a query only _fetching_ some of the table columns, which in practice only
+ * happens for thrift queries, and thrift queries shouldn't mess up with CQL tables and collection tombstones,
+ * so this test is not of the utmost importance. Nonetheless, the pre-CASSANDRA-15805 behavior was incorrect and
+ * this ensure it is fixed.
+ */
+ @Test
+ public void testCellGrouperOnNonFecthedCollectionTombstone()
+ {
+ // CREATE TABLE %s (pk int, ck int, a text, b set<text>, c text, PRIMARY KEY (pk, ck))
+ CFMetaData cfm = CFMetaData.Builder.create("ks", "table")
+ .addPartitionKey("pk", Int32Type.instance)
+ .addClusteringColumn("ck", Int32Type.instance)
+ .addRegularColumn("a", UTF8Type.instance)
+ .addRegularColumn("b", SetType.getInstance(UTF8Type.instance, true))
+ .addRegularColumn("c", UTF8Type.instance)
+ .build();
+
+ // Creates a filter that _only_ fetches a and c, but not b.
+ ColumnFilter filter = ColumnFilter.selectionBuilder()
+ .add(cfm.getColumnDefinition(bytes("a")))
+ .add(cfm.getColumnDefinition(bytes("c")))
+ .build();
+ SerializationHelper helper = new SerializationHelper(cfm,
+ MessagingService.VERSION_22,
+ SerializationHelper.Flag.LOCAL,
+ filter);
+ CellGrouper grouper = new CellGrouper(cfm, helper);
+ Clustering clustering = new Clustering(bytes(1));
+
+ // We add a cell for a, then a collection tombstone for b, and then a cell for c (for the same clustering).
+ // All those additions should return 'true' as all belong to the same row.
+ LegacyCell ca = cell(clustering, cfm.getColumnDefinition(bytes("a")), bytes("v1"), 1);
+ assertTrue(grouper.addAtom(ca));
+
+ Slice.Bound startBound = Slice.Bound.inclusiveStartOf(bytes(1));
+ Slice.Bound endBound = Slice.Bound.inclusiveEndOf(bytes(1));
+ ColumnDefinition bDef = cfm.getColumnDefinition(bytes("b"));
+ assert bDef != null;
+ LegacyBound start = new LegacyBound(startBound, false, bDef);
+ LegacyBound end = new LegacyBound(endBound, false, bDef);
+ LegacyRangeTombstone rtb = new LegacyRangeTombstone(start, end, new DeletionTime(1, 1588598040));
+ assertTrue(rtb.isCollectionTombstone()); // Ensure we're testing what we think
+ assertTrue(grouper.addAtom(rtb));
+
+ LegacyCell cc = cell(clustering, cfm.getColumnDefinition(bytes("c")), bytes("v2"), 1);
+ assertTrue(grouper.addAtom(cc));
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org