Dropped columns can cause reverse sstable iteration to return prematurely Patch by Blake Eggleston; Reviewed by Sam Tunnicliffe for CASSANDRA-14838
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/e4bac44a Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/e4bac44a Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/e4bac44a Branch: refs/heads/cassandra-3.11 Commit: e4bac44a04d59d93f622d91ef40b462250dac613 Parents: e07d53a Author: Blake Eggleston <bdeggles...@gmail.com> Authored: Tue Oct 23 12:46:34 2018 -0700 Committer: Blake Eggleston <bdeggles...@gmail.com> Committed: Sun Oct 28 19:29:44 2018 -0700 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../columniterator/SSTableReversedIterator.java | 9 +- .../SSTableReverseIteratorTest.java | 98 ++++++++++++++++++++ 3 files changed, 103 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/e4bac44a/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 78c0c47..cc8e348 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.0.18 + * Dropped columns can cause reverse sstable iteration to return prematurely (CASSANDRA-14838) * Legacy sstables with multi block range tombstones create invalid bound sequences (CASSANDRA-14823) * Expand range tombstone validation checks to multiple interim request stages (CASSANDRA-14824) * Reverse order reads can return incomplete results (CASSANDRA-14803) http://git-wip-us.apache.org/repos/asf/cassandra/blob/e4bac44a/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java b/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java index 2d95dab..8d3f4f3 100644 --- a/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java +++ b/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java @@ -404,19 +404,18 @@ public class SSTableReversedIterator extends AbstractSSTableIterator indexState.setToBlock(nextBlockIdx); readCurrentBlock(true, nextBlockIdx != lastBlockIdx); - // for pre-3.0 storage formats, index blocks that only contain a single row and that row crosses + // If an indexed block only contains data for a dropped column, the iterator will be empty, even + // though we may still have data to read in subsequent blocks + + // also, for pre-3.0 storage formats, index blocks that only contain a single row and that row crosses // index boundaries, the iterator will be empty even though we haven't read everything we're intending // to read. In that case, we want to read the next index block. This shouldn't be possible in 3.0+ // formats (see next comment) if (!iterator.hasNext() && nextBlockIdx > lastBlockIdx) { - Verify.verify(!sstable.descriptor.version.storeRows()); continue; } - // for 3.0+ storage formats, since that new block is within the bounds we've computed in setToSlice(), - // we know there will always be something matching the slice unless we're on the lastBlockIdx (in which - // case there may or may not be results, but if there isn't, we're done for the slice). return iterator.hasNext(); } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/e4bac44a/test/unit/org/apache/cassandra/db/columniterator/SSTableReverseIteratorTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/db/columniterator/SSTableReverseIteratorTest.java b/test/unit/org/apache/cassandra/db/columniterator/SSTableReverseIteratorTest.java new file mode 100644 index 0000000..2f183c0 --- /dev/null +++ b/test/unit/org/apache/cassandra/db/columniterator/SSTableReverseIteratorTest.java @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.db.columniterator; + +import java.nio.ByteBuffer; +import java.util.Random; + +import com.google.common.collect.Iterables; +import org.junit.Assert; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.SchemaLoader; +import org.apache.cassandra.cql3.QueryProcessor; +import org.apache.cassandra.cql3.UntypedResultSet; +import org.apache.cassandra.db.ColumnFamilyStore; +import org.apache.cassandra.db.DecoratedKey; +import org.apache.cassandra.db.Keyspace; +import org.apache.cassandra.db.RowIndexEntry; +import org.apache.cassandra.db.marshal.Int32Type; +import org.apache.cassandra.io.sstable.format.SSTableReader; +import org.apache.cassandra.schema.KeyspaceParams; + +public class SSTableReverseIteratorTest +{ + private static final String KEYSPACE = "ks"; + private Random random; + + @BeforeClass + public static void setupClass() + { + SchemaLoader.prepareServer(); + SchemaLoader.createKeyspace(KEYSPACE, KeyspaceParams.simple(1)); + } + + @Before + public void setUp() + { + random = new Random(0); + } + + private ByteBuffer bytes(int size) + { + byte[] b = new byte[size]; + random.nextBytes(b); + return ByteBuffer.wrap(b); + } + + /** + * SSTRI shouldn't bail out if it encounters empty blocks (due to dropped columns) + */ + @Test + public void emptyBlockTolerance() + { + String table = "empty_block_tolerance"; + QueryProcessor.executeInternal(String.format("CREATE TABLE %s.%s (k INT, c int, v1 blob, v2 blob, primary key (k, c))", KEYSPACE, table)); + ColumnFamilyStore tbl = Keyspace.open(KEYSPACE).getColumnFamilyStore(table); + assert tbl != null; + + int key = 100; + + QueryProcessor.executeInternal(String.format("UPDATE %s.%s SET v1=?, v2=? WHERE k=? AND c=?", KEYSPACE, table), bytes(8), bytes(8), key, 0); + QueryProcessor.executeInternal(String.format("UPDATE %s.%s SET v1=? WHERE k=? AND c=?", KEYSPACE, table), bytes(0x20000), key, 1); + QueryProcessor.executeInternal(String.format("UPDATE %s.%s SET v1=? WHERE k=? AND c=?", KEYSPACE, table), bytes(0x20000), key, 2); + QueryProcessor.executeInternal(String.format("UPDATE %s.%s SET v1=? WHERE k=? AND c=?", KEYSPACE, table), bytes(0x20000), key, 3); + + tbl.forceBlockingFlush(); + SSTableReader sstable = Iterables.getOnlyElement(tbl.getLiveSSTables()); + DecoratedKey dk = tbl.getPartitioner().decorateKey(Int32Type.instance.decompose(key)); + RowIndexEntry indexEntry = sstable.getPosition(dk, SSTableReader.Operator.EQ); + Assert.assertTrue(indexEntry.isIndexed()); + Assert.assertTrue(indexEntry.columnsIndex().size() > 2); + + // drop v1 so the first 2 index blocks only contain empty unfiltereds + QueryProcessor.executeInternal(String.format("ALTER TABLE %s.%s DROP v1", KEYSPACE, table)); + + UntypedResultSet result = QueryProcessor.executeInternal(String.format("SELECT v2 FROM %s.%s WHERE k=? ORDER BY c DESC", KEYSPACE, table), key); + Assert.assertEquals(1, result.size()); + + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
