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 <lebre...@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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org