From c843b6b85cf828fde16d8d8a04411cba515f715e Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Fri, 21 Mar 2014 19:03:12 +0100 Subject: [PATCH] Fix paging with SELECT DISTINCT patch by slebresne; reviewed by thobbs for CASSANDRA-6857 --- CHANGES.txt | 1 + .../org/apache/cassandra/db/ColumnFamilyStore.java | 3 ++- .../org/apache/cassandra/db/PagedRangeCommand.java | 12 ++++++++++-- .../apache/cassandra/service/pager/QueryPagers.java | 10 ++++++++-- .../apache/cassandra/db/ColumnFamilyStoreTest.java | 4 ++-- 5 files changed, 23 insertions(+), 7 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 31fd319234..c5f2666d13 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -25,6 +25,7 @@ * Extend triggers to support CAS updates (CASSANDRA-6882) * Static columns with IF NOT EXISTS don't always work as expected (CASSANDRA-6873) * Add CqlRecordReader to take advantage of native CQL pagination (CASSANDRA-6311) + * Fix paging with SELECT DISTINCT (CASSANDRA-6857) Merged from 1.2: * Add UNLOGGED, COUNTER options to BATCH documentation (CASSANDRA-6816) * add extra SSL cipher suites (CASSANDRA-6613) diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index 5c3eb19621..b58329e7db 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -1671,10 +1671,11 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean ByteBuffer columnStop, List rowFilter, int maxResults, + boolean countCQL3Rows, long now) { DataRange dataRange = new DataRange.Paging(keyRange, columnRange, columnStart, columnStop, metadata.comparator); - return ExtendedFilter.create(this, dataRange, rowFilter, maxResults, true, now); + return ExtendedFilter.create(this, dataRange, rowFilter, maxResults, countCQL3Rows, now); } public List getRangeSlice(AbstractBounds range, diff --git a/src/java/org/apache/cassandra/db/PagedRangeCommand.java b/src/java/org/apache/cassandra/db/PagedRangeCommand.java index e152f436b5..d6f3ca10ea 100644 --- a/src/java/org/apache/cassandra/db/PagedRangeCommand.java +++ b/src/java/org/apache/cassandra/db/PagedRangeCommand.java @@ -97,14 +97,22 @@ public class PagedRangeCommand extends AbstractRangeCommand public boolean countCQL3Rows() { - return true; + // We only use PagedRangeCommand for CQL3. However, for SELECT DISTINCT, we want to return false here, because + // we just want to pick the first cell of each partition and returning true here would throw off the logic in + // ColumnFamilyStore.filter(). + // What we do know is that for a SELECT DISTINCT the underlying SliceQueryFilter will have a compositesToGroup==-1 + // and a count==1. And while it would be possible for a normal SELECT on a COMPACT table to also have such + // parameters, it's fine returning false since if we do count one cell for each partition, then each partition + // will coincide with exactly one CQL3 row. + SliceQueryFilter filter = (SliceQueryFilter)predicate; + return filter.compositesToGroup >= 0 || filter.count != 1; } public List executeLocally() { ColumnFamilyStore cfs = Keyspace.open(keyspace).getColumnFamilyStore(columnFamily); - ExtendedFilter exFilter = cfs.makeExtendedFilter(keyRange, (SliceQueryFilter)predicate, start, stop, rowFilter, limit, timestamp); + ExtendedFilter exFilter = cfs.makeExtendedFilter(keyRange, (SliceQueryFilter)predicate, start, stop, rowFilter, limit, countCQL3Rows(), timestamp); if (cfs.indexManager.hasIndexFor(rowFilter)) return cfs.search(exFilter); else diff --git a/src/java/org/apache/cassandra/service/pager/QueryPagers.java b/src/java/org/apache/cassandra/service/pager/QueryPagers.java index c353536e72..65112aaf77 100644 --- a/src/java/org/apache/cassandra/service/pager/QueryPagers.java +++ b/src/java/org/apache/cassandra/service/pager/QueryPagers.java @@ -71,8 +71,14 @@ public class QueryPagers else { assert command instanceof RangeSliceCommand; - // We can never be sure a range slice won't need paging - return true; + RangeSliceCommand rsc = (RangeSliceCommand)command; + // We don't support paging for thrift in general because the way thrift RangeSliceCommand count rows + // independently of cells makes things harder (see RangeSliceQueryPager). The one case where we do + // get a RangeSliceCommand from CQL3 without the countCQL3Rows flag set is for DISTINCT. In that case + // however, the underlying sliceQueryFilter count is 1, so that the RSC limit is still a limit on the + // number of CQL3 rows returned. + assert rsc.countCQL3Rows || (rsc.predicate instanceof SliceQueryFilter && ((SliceQueryFilter)rsc.predicate).count == 1); + return rsc.maxResults > pageSize; } } diff --git a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java index 5fc006b38f..bc39ad6c13 100644 --- a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java +++ b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java @@ -1198,7 +1198,7 @@ public class ColumnFamilyStoreTest extends SchemaLoader ByteBufferUtil.bytes("c2"), false, 0); - rows = cfs.getRangeSlice(cfs.makeExtendedFilter(new Bounds(ka, kc), sf, ByteBufferUtil.bytes("c2"), ByteBufferUtil.bytes("c1"), null, 2, System.currentTimeMillis())); + rows = cfs.getRangeSlice(cfs.makeExtendedFilter(new Bounds(ka, kc), sf, ByteBufferUtil.bytes("c2"), ByteBufferUtil.bytes("c1"), null, 2, true, System.currentTimeMillis())); assert rows.size() == 2 : "Expected 2 rows, got " + toString(rows); iter = rows.iterator(); row1 = iter.next(); @@ -1206,7 +1206,7 @@ public class ColumnFamilyStoreTest extends SchemaLoader assertColumnNames(row1, "c2"); assertColumnNames(row2, "c1"); - rows = cfs.getRangeSlice(cfs.makeExtendedFilter(new Bounds(kb, kc), sf, ByteBufferUtil.bytes("c1"), ByteBufferUtil.bytes("c1"), null, 10, System.currentTimeMillis())); + rows = cfs.getRangeSlice(cfs.makeExtendedFilter(new Bounds(kb, kc), sf, ByteBufferUtil.bytes("c1"), ByteBufferUtil.bytes("c1"), null, 10, true, System.currentTimeMillis())); assert rows.size() == 2 : "Expected 2 rows, got " + toString(rows); iter = rows.iterator(); row1 = iter.next();