Updated Branches: refs/heads/trunk 0619da2ae -> cb871ba90
Fix paging with reversed slices patch by slebresne; reviewed by iamalesksey for CASSANDRA-6343 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/5008507c Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/5008507c Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/5008507c Branch: refs/heads/trunk Commit: 5008507ca21265e0c6be53d61024baa7eaf187fc Parents: b457156 Author: Sylvain Lebresne <sylv...@datastax.com> Authored: Wed Nov 13 17:59:07 2013 +0100 Committer: Sylvain Lebresne <sylv...@datastax.com> Committed: Wed Nov 13 17:59:07 2013 +0100 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../org/apache/cassandra/db/ColumnFamily.java | 5 ++ .../service/pager/AbstractQueryPager.java | 49 ++++++++++++++------ .../service/pager/RangeNamesQueryPager.java | 5 ++ .../service/pager/RangeSliceQueryPager.java | 9 +++- .../service/pager/SliceQueryPager.java | 9 +++- .../cassandra/service/QueryPagerTest.java | 32 ++++++++++++- 7 files changed, 92 insertions(+), 18 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/5008507c/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index e4f1862..159e8de 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -20,6 +20,7 @@ * Fix serialization bug in PagedRange with 2ndary indexes (CASSANDRA-6299) * Fix CQL3 table validation in Thrift (CASSANDRA-6140) * Fix bug missing results with IN clauses (CASSANDRA-6327) + * Fix paging with reversed slices (CASSANDRA-6343) Merged from 1.2: * add non-jamm path for cached statements (CASSANDRA-6293) * (Hadoop) Require CFRR batchSize to be at least 2 (CASSANDRA-6114) http://git-wip-us.apache.org/repos/asf/cassandra/blob/5008507c/src/java/org/apache/cassandra/db/ColumnFamily.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/ColumnFamily.java b/src/java/org/apache/cassandra/db/ColumnFamily.java index b2c5ac4..47b14b9 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamily.java +++ b/src/java/org/apache/cassandra/db/ColumnFamily.java @@ -451,6 +451,11 @@ public abstract class ColumnFamily implements Iterable<Column>, IRowCacheEntry return getSortedColumns().iterator(); } + public Iterator<Column> reverseIterator() + { + return getReverseSortedColumns().iterator(); + } + public boolean hasIrrelevantData(int gcBefore) { // Do we have gcable deletion infos? http://git-wip-us.apache.org/repos/asf/cassandra/blob/5008507c/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 62cd454..d040203 100644 --- a/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java +++ b/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java @@ -93,7 +93,7 @@ abstract class AbstractQueryPager implements QueryPager remaining++; } // Otherwise, if 'lastWasRecorded', we queried for one more than the page size, - // so if the page was is full, trim the last entry + // so if the page is full, trim the last entry else if (lastWasRecorded && !exhausted) { // We've asked for one more than necessary @@ -161,11 +161,14 @@ abstract class AbstractQueryPager implements QueryPager protected abstract List<Row> queryNextPage(int pageSize, ConsistencyLevel consistency, boolean localQuery) throws RequestValidationException, RequestExecutionException; protected abstract boolean containsPreviousLast(Row first); protected abstract boolean recordLast(Row last); + protected abstract boolean isReversed(); private List<Row> discardFirst(List<Row> rows) { Row first = rows.get(0); - ColumnFamily newCf = discardFirst(first.cf); + ColumnFamily newCf = isReversed() + ? discardLast(first.cf) + : discardFirst(first.cf); int count = newCf.getColumnCount(); List<Row> newRows = new ArrayList<Row>(count == 0 ? rows.size() - 1 : rows.size()); @@ -179,7 +182,9 @@ abstract class AbstractQueryPager implements QueryPager private List<Row> discardLast(List<Row> rows) { Row last = rows.get(rows.size() - 1); - ColumnFamily newCf = discardLast(last.cf); + ColumnFamily newCf = isReversed() + ? discardFirst(last.cf) + : discardLast(last.cf); int count = newCf.getColumnCount(); List<Row> newRows = new ArrayList<Row>(count == 0 ? rows.size() - 1 : rows.size()); @@ -200,11 +205,27 @@ abstract class AbstractQueryPager implements QueryPager private ColumnFamily discardFirst(ColumnFamily cf) { + boolean isReversed = isReversed(); + DeletionInfo.InOrderTester tester = cf.deletionInfo().inOrderTester(isReversed); + return isReversed + ? discardTail(cf, cf.reverseIterator(), tester) + : discardHead(cf, cf.iterator(), tester); + } + + private ColumnFamily discardLast(ColumnFamily cf) + { + boolean isReversed = isReversed(); + DeletionInfo.InOrderTester tester = cf.deletionInfo().inOrderTester(isReversed); + return isReversed + ? discardHead(cf, cf.reverseIterator(), tester) + : discardTail(cf, cf.iterator(), tester); + } + + private ColumnFamily discardHead(ColumnFamily cf, Iterator<Column> iter, DeletionInfo.InOrderTester tester) + { ColumnFamily copy = cf.cloneMeShallow(); ColumnCounter counter = columnCounter(); - Iterator<Column> iter = cf.iterator(); - DeletionInfo.InOrderTester tester = cf.inOrderDeletionTester(); // Discard the first live while (iter.hasNext()) { @@ -220,22 +241,24 @@ abstract class AbstractQueryPager implements QueryPager return copy; } - private ColumnFamily discardLast(ColumnFamily cf) + private ColumnFamily discardTail(ColumnFamily cf, Iterator<Column> iter, DeletionInfo.InOrderTester tester) { ColumnFamily copy = cf.cloneMeShallow(); - // Redoing the counting like that is not extremely efficient, but - // discardLast is only called in case of a race between paging and - // a deletion, which is pretty unlikely, so probably not a big deal + // Redoing the counting like that is not extremely efficient. + // This is called only for reversed slices or in the case of a race between + // paging and a deletion (pretty unlikely), so this is probably acceptable. int liveCount = columnCounter().countAll(cf).live(); ColumnCounter counter = columnCounter(); - DeletionInfo.InOrderTester tester = cf.inOrderDeletionTester(); // Discard the first live - for (Column c : cf) + while (iter.hasNext()) { + Column c = iter.next(); counter.count(c, tester); - if (counter.live() < liveCount) - copy.addColumn(c); + if (counter.live() >= liveCount) + break; + + copy.addColumn(c); } return copy; } http://git-wip-us.apache.org/repos/asf/cassandra/blob/5008507c/src/java/org/apache/cassandra/service/pager/RangeNamesQueryPager.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/pager/RangeNamesQueryPager.java b/src/java/org/apache/cassandra/service/pager/RangeNamesQueryPager.java index 57fb05b..e3b0cf8 100644 --- a/src/java/org/apache/cassandra/service/pager/RangeNamesQueryPager.java +++ b/src/java/org/apache/cassandra/service/pager/RangeNamesQueryPager.java @@ -91,6 +91,11 @@ public class RangeNamesQueryPager extends AbstractQueryPager return false; } + protected boolean isReversed() + { + return false; + } + private AbstractBounds<RowPosition> makeExcludingKeyBounds(RowPosition lastReturnedKey) { // We return a range that always exclude lastReturnedKey, since we've already http://git-wip-us.apache.org/repos/asf/cassandra/blob/5008507c/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 42a9585..1f4ba78 100644 --- a/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java +++ b/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java @@ -91,16 +91,21 @@ public class RangeSliceQueryPager extends AbstractQueryPager { return lastReturnedKey != null && lastReturnedKey.equals(first.key) - && lastReturnedName.equals(firstName(first.cf)); + && lastReturnedName.equals(isReversed() ? lastName(first.cf) : firstName(first.cf)); } protected boolean recordLast(Row last) { lastReturnedKey = last.key; - lastReturnedName = lastName(last.cf); + lastReturnedName = isReversed() ? firstName(last.cf) : lastName(last.cf); return true; } + protected boolean isReversed() + { + return ((SliceQueryFilter)command.predicate).reversed; + } + private AbstractBounds<RowPosition> makeIncludingKeyBounds(RowPosition lastReturnedKey) { // We always include lastReturnedKey since we may still be paging within a row, http://git-wip-us.apache.org/repos/asf/cassandra/blob/5008507c/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 1d77144..e3825a9 100644 --- a/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java +++ b/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java @@ -76,12 +76,17 @@ public class SliceQueryPager extends AbstractQueryPager implements SinglePartiti protected boolean containsPreviousLast(Row first) { - return lastReturned != null && lastReturned.equals(firstName(first.cf)); + return lastReturned != null && lastReturned.equals(isReversed() ? lastName(first.cf) : firstName(first.cf)); } protected boolean recordLast(Row last) { - lastReturned = lastName(last.cf); + lastReturned = isReversed() ? firstName(last.cf) : lastName(last.cf); return true; } + + protected boolean isReversed() + { + return command.filter.reversed; + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/5008507c/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 3fc2ac2..f395cf4 100644 --- a/test/unit/org/apache/cassandra/service/QueryPagerTest.java +++ b/test/unit/org/apache/cassandra/service/QueryPagerTest.java @@ -117,7 +117,12 @@ public class QueryPagerTest extends SchemaLoader private static ReadCommand sliceQuery(String key, String start, String end, int count) { - SliceQueryFilter filter = new SliceQueryFilter(bytes(start), bytes(end), false, count); + return sliceQuery(key, start, end, false, count); + } + + private static ReadCommand sliceQuery(String key, String start, String end, boolean reversed, int count) + { + SliceQueryFilter filter = new SliceQueryFilter(bytes(start), bytes(end), reversed, count); // Note: for MultiQueryTest, we need the same timestamp/expireBefore for all queries, so we just use 0 as it doesn't matter here. return new SliceFromReadCommand(KS, bytes(key), CF, 0, filter); } @@ -188,6 +193,31 @@ public class QueryPagerTest extends SchemaLoader } @Test + public void reversedSliceQueryTest() throws Exception + { + QueryPager pager = QueryPagers.localPager(sliceQuery("k0", "c8", "c1", true, 10)); + + List<Row> page; + + assertFalse(pager.isExhausted()); + page = pager.fetchPage(3); + assertEquals(toString(page), 1, page.size()); + assertRow(page.get(0), "k0", "c6", "c7", "c8"); + + assertFalse(pager.isExhausted()); + page = pager.fetchPage(3); + assertEquals(toString(page), 1, page.size()); + assertRow(page.get(0), "k0", "c3", "c4", "c5"); + + assertFalse(pager.isExhausted()); + page = pager.fetchPage(3); + assertEquals(toString(page), 1, page.size()); + assertRow(page.get(0), "k0", "c1", "c2"); + + assertTrue(pager.isExhausted()); + } + + @Test public void MultiQueryTest() throws Exception { QueryPager pager = QueryPagers.localPager(new Pageable.ReadCommands(new ArrayList<ReadCommand>() {{
