Repository: cassandra Updated Branches: refs/heads/cassandra-2.1 6f4e0ba6b -> 3d9305320
Fix paging bug with deleted columns patch by slebresne; reviewed by iamaleksey for CASSANDRA-6748 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/cd2c4388 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/cd2c4388 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/cd2c4388 Branch: refs/heads/cassandra-2.1 Commit: cd2c43884d600f9d444fbacd05f56c3581c10aa0 Parents: 3bfb764 Author: Sylvain Lebresne <[email protected]> Authored: Tue Feb 25 10:31:14 2014 +0100 Committer: Sylvain Lebresne <[email protected]> Committed: Tue Feb 25 10:37:18 2014 +0100 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../service/pager/AbstractQueryPager.java | 8 ++-- .../service/pager/RangeSliceQueryPager.java | 13 ++++-- .../service/pager/SliceQueryPager.java | 13 +++++- .../unit/org/apache/cassandra/SchemaLoader.java | 9 +++- .../cassandra/service/QueryPagerTest.java | 44 ++++++++++++++++++-- 6 files changed, 74 insertions(+), 14 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index bfcb6a4..f3a854c 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -22,6 +22,7 @@ * Add static columns to CQL3 (CASSANDRA-6561) * Optimize single partition batch statements (CASSANDRA-6737) * Disallow post-query re-ordering when paging (CASSANDRA-6722) + * Fix potential paging bug with deleted columns (CASSANDRA-6748) Merged from 1.2: * Catch memtable flush exceptions during shutdown (CASSANDRA-6735) * Fix broken streams when replacing with same IP (CASSANDRA-6622) http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java b/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java index 297a85f..1b4bdbd 100644 --- a/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java +++ b/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java @@ -332,13 +332,13 @@ abstract class AbstractQueryPager implements QueryPager return Math.min(liveCount, toDiscard); } - protected static ByteBuffer firstName(ColumnFamily cf) + protected static Column firstColumn(ColumnFamily cf) { - return cf.iterator().next().name(); + return cf.iterator().next(); } - protected static ByteBuffer lastName(ColumnFamily cf) + protected static Column lastColumn(ColumnFamily cf) { - return cf.getReverseSortedColumns().iterator().next().name(); + return cf.getReverseSortedColumns().iterator().next(); } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java b/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java index 1f4ba78..0df1d25 100644 --- a/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java +++ b/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java @@ -89,15 +89,20 @@ public class RangeSliceQueryPager extends AbstractQueryPager protected boolean containsPreviousLast(Row first) { - return lastReturnedKey != null - && lastReturnedKey.equals(first.key) - && lastReturnedName.equals(isReversed() ? lastName(first.cf) : firstName(first.cf)); + if (lastReturnedKey == null || !lastReturnedKey.equals(first.key)) + return false; + + // Same as SliceQueryPager, we ignore a deleted column + Column firstColumn = isReversed() ? lastColumn(first.cf) : firstColumn(first.cf); + return !first.cf.deletionInfo().isDeleted(firstColumn) + && firstColumn.isLive(timestamp()) + && lastReturnedName.equals(firstColumn.name()); } protected boolean recordLast(Row last) { lastReturnedKey = last.key; - lastReturnedName = isReversed() ? firstName(last.cf) : lastName(last.cf); + lastReturnedName = (isReversed() ? firstColumn(last.cf) : lastColumn(last.cf)).name(); return true; } http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java b/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java index cd0c069..c94f7f6 100644 --- a/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java +++ b/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java @@ -81,12 +81,21 @@ public class SliceQueryPager extends AbstractQueryPager implements SinglePartiti protected boolean containsPreviousLast(Row first) { - return lastReturned != null && lastReturned.equals(isReversed() ? lastName(first.cf) : firstName(first.cf)); + if (lastReturned == null) + return false; + + Column firstColumn = isReversed() ? lastColumn(first.cf) : firstColumn(first.cf); + // Note: we only return true if the column is the lastReturned *and* it is live. If it is deleted, it is ignored by the + // rest of the paging code (it hasn't been counted as live in particular) and we want to act as if it wasn't there. + return !first.cf.deletionInfo().isDeleted(firstColumn) + && firstColumn.isLive(timestamp()) + && lastReturned.equals(firstColumn.name()); } protected boolean recordLast(Row last) { - lastReturned = isReversed() ? firstName(last.cf) : lastName(last.cf); + Column lastColumn = isReversed() ? firstColumn(last.cf) : lastColumn(last.cf); + lastReturned = lastColumn.name(); return true; } http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/test/unit/org/apache/cassandra/SchemaLoader.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/SchemaLoader.java b/test/unit/org/apache/cassandra/SchemaLoader.java index 58cc52f..d554a8c 100644 --- a/test/unit/org/apache/cassandra/SchemaLoader.java +++ b/test/unit/org/apache/cassandra/SchemaLoader.java @@ -300,7 +300,14 @@ public class SchemaLoader + "k int PRIMARY KEY," + "v1 text," + "v2 int" - + ")", ks_cql))); + + ")", ks_cql), + + CFMetaData.compile("CREATE TABLE table2 (" + + "k text," + + "c text," + + "v text," + + "PRIMARY KEY (k, c))", ks_cql) + )); if (Boolean.parseBoolean(System.getProperty("cassandra.test.compression", "false"))) http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/test/unit/org/apache/cassandra/service/QueryPagerTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/service/QueryPagerTest.java b/test/unit/org/apache/cassandra/service/QueryPagerTest.java index f395cf4..0645433 100644 --- a/test/unit/org/apache/cassandra/service/QueryPagerTest.java +++ b/test/unit/org/apache/cassandra/service/QueryPagerTest.java @@ -31,11 +31,13 @@ import org.apache.cassandra.SchemaLoader; import org.apache.cassandra.OrderedJUnit4ClassRunner; import org.apache.cassandra.db.*; import org.apache.cassandra.db.filter.*; +import org.apache.cassandra.db.marshal.CompositeType; import org.apache.cassandra.dht.*; import org.apache.cassandra.service.pager.*; import org.apache.cassandra.utils.ByteBufferUtil; import static org.junit.Assert.*; +import static org.apache.cassandra.cql3.QueryProcessor.processInternal; import static org.apache.cassandra.Util.range; import static org.apache.cassandra.utils.ByteBufferUtil.bytes; @@ -143,14 +145,25 @@ public class QueryPagerTest extends SchemaLoader private static void assertRow(Row r, String key, String... names) { + ByteBuffer[] bbs = new ByteBuffer[names.length]; + for (int i = 0; i < names.length; i++) + bbs[i] = bytes(names[i]); + assertRow(r, key, bbs); + } + + private static void assertRow(Row r, String key, ByteBuffer... names) + { assertEquals(key, string(r.key.key)); assertNotNull(r.cf); - assertEquals(toString(r.cf), names.length, r.cf.getColumnCount()); int i = 0; for (Column c : r.cf) { - String expected = names[i++]; - assertEquals("column " + i + " doesn't match: " + toString(r.cf), expected, string(c.name())); + // Ignore deleted cells if we have them + if (!c.isLive(0)) + continue; + + ByteBuffer expected = names[i++]; + assertEquals("column " + i + " doesn't match: " + toString(r.cf), expected, c.name()); } } @@ -310,4 +323,29 @@ public class QueryPagerTest extends SchemaLoader assertTrue(pager.isExhausted()); } + + @Test + public void SliceQueryWithTombstoneTest() throws Exception + { + // Testing for the bug of #6748 + String keyspace = "cql_keyspace"; + String table = "table2"; + ColumnFamilyStore cfs = Keyspace.open(keyspace).getColumnFamilyStore(table); + CompositeType ct = (CompositeType)cfs.metadata.comparator; + + // Insert rows but with a tombstone as last cell + for (int i = 0; i < 5; i++) + processInternal(String.format("INSERT INTO %s.%s (k, c, v) VALUES ('k%d', 'c%d', null)", keyspace, table, 0, i)); + + SliceQueryFilter filter = new SliceQueryFilter(ColumnSlice.ALL_COLUMNS_ARRAY, false, 100); + QueryPager pager = QueryPagers.localPager(new SliceFromReadCommand(keyspace, bytes("k0"), table, 0, filter)); + + for (int i = 0; i < 5; i++) + { + List<Row> page = pager.fetchPage(1); + assertEquals(toString(page), 1, page.size()); + // The only live cell we should have each time is the row marker + assertRow(page.get(0), "k0", ct.decompose("c" + i, "")); + } + } }
