You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by sl...@apache.org on 2015/12/23 12:23:08 UTC
[1/2] cassandra git commit: Fix assertion error when reading static
on an indexed sstable
Repository: cassandra
Updated Branches:
refs/heads/10903 [created] e7c6e14fd
Fix assertion error when reading static on an indexed sstable
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/bc887acf
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/bc887acf
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/bc887acf
Branch: refs/heads/10903
Commit: bc887acfaec939efb5b9abeb3b7e7bcf4c1c3870
Parents: 11165f4
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Mon Dec 21 12:14:46 2015 +0100
Committer: Sylvain Lebresne <sy...@datastax.com>
Committed: Mon Dec 21 13:41:11 2015 +0100
----------------------------------------------------------------------
.../columniterator/AbstractSSTableIterator.java | 33 ++++----
.../db/columniterator/SSTableIterator.java | 38 +++-------
.../columniterator/SSTableReversedIterator.java | 28 ++-----
.../cql3/QueryWithIndexedSSTableTest.java | 80 ++++++++++++++++++++
4 files changed, 112 insertions(+), 67 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cassandra/blob/bc887acf/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java b/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
index 5f280d7..f103ee2 100644
--- a/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
+++ b/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
@@ -99,14 +99,14 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
// Note that this needs to be called after file != null and after the partitionDeletion has been set, but before readStaticRow
// (since it uses it) so we can't move that up (but we'll be able to simplify as soon as we drop support for the old file format).
- this.reader = needsReader ? createReader(indexEntry, file, true, shouldCloseFile) : null;
+ this.reader = needsReader ? createReader(indexEntry, file, shouldCloseFile) : null;
this.staticRow = readStaticRow(sstable, file, helper, columns.fetchedColumns().statics, isForThrift, reader == null ? null : reader.deserializer);
}
else
{
this.partitionLevelDeletion = indexEntry.deletionTime();
this.staticRow = Rows.EMPTY_STATIC_ROW;
- this.reader = needsReader ? createReader(indexEntry, file, false, shouldCloseFile) : null;
+ this.reader = needsReader ? createReader(indexEntry, file, shouldCloseFile) : null;
}
if (reader == null && file != null && shouldCloseFile)
@@ -180,7 +180,7 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
}
}
- protected abstract Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean isAtPartitionStart, boolean shouldCloseFile);
+ protected abstract Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean shouldCloseFile);
public CFMetaData metadata()
{
@@ -291,19 +291,13 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
// Records the currently open range tombstone (if any)
protected DeletionTime openMarker = null;
- // !isInit means we have never seeked in the file and thus should seek before reading anything
- protected boolean isInit;
-
- protected Reader(FileDataInput file, boolean isInit, boolean shouldCloseFile)
+ protected Reader(FileDataInput file, boolean shouldCloseFile)
{
this.file = file;
- this.isInit = isInit;
this.shouldCloseFile = shouldCloseFile;
if (file != null)
createDeserializer();
- else
- assert !isInit;
}
private void createDeserializer()
@@ -343,12 +337,6 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
{
try
{
- if (!isInit)
- {
- init();
- isInit = true;
- }
-
return hasNextInternal();
}
catch (IOException e)
@@ -387,9 +375,6 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
}
}
- // Called is hasNext() is called but we haven't been yet initialized
- protected abstract void init() throws IOException;
-
// Set the reader so its hasNext/next methods return values within the provided slice
public abstract void setForSlice(Slice slice) throws IOException;
@@ -460,7 +445,15 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
// Update the block idx based on the current reader position if we're past the current block.
public void updateBlock() throws IOException
{
- assert currentIndexIdx >= 0;
+ // If we get here with currentBlockIdx < 0, it means setToBlock() has never been called, so it means
+ // we're about to read from the beginning of the partition, but haven't "prepared" the IndexState yet.
+ // Do so by setting us on the first block.
+ if (currentIndexIdx < 0)
+ {
+ setToBlock(0);
+ return;
+ }
+
while (currentIndexIdx + 1 < indexes.size() && isPastCurrentBlock())
{
reader.openMarker = currentIndex().endOpenMarker;
http://git-wip-us.apache.org/repos/asf/cassandra/blob/bc887acf/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java b/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java
index 3536d65..0409310 100644
--- a/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java
+++ b/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java
@@ -46,11 +46,11 @@ public class SSTableIterator extends AbstractSSTableIterator
super(sstable, file, key, indexEntry, columns, isForThrift);
}
- protected Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean isAtPartitionStart, boolean shouldCloseFile)
+ protected Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean shouldCloseFile)
{
return indexEntry.isIndexed()
- ? new ForwardIndexedReader(indexEntry, file, isAtPartitionStart, shouldCloseFile)
- : new ForwardReader(file, isAtPartitionStart, shouldCloseFile);
+ ? new ForwardIndexedReader(indexEntry, file, shouldCloseFile)
+ : new ForwardReader(file, shouldCloseFile);
}
public boolean isReverseOrder()
@@ -70,16 +70,9 @@ public class SSTableIterator extends AbstractSSTableIterator
protected boolean sliceDone; // set to true once we know we have no more result for the slice. This is in particular
// used by the indexed reader when we know we can't have results based on the index.
- private ForwardReader(FileDataInput file, boolean isAtPartitionStart, boolean shouldCloseFile)
+ private ForwardReader(FileDataInput file, boolean shouldCloseFile)
{
- super(file, isAtPartitionStart, shouldCloseFile);
- }
-
- protected void init() throws IOException
- {
- // We should always have been initialized (at the beginning of the partition). Only indexed readers may
- // have to initialize.
- throw new IllegalStateException();
+ super(file, shouldCloseFile);
}
public void setForSlice(Slice slice) throws IOException
@@ -95,6 +88,8 @@ public class SSTableIterator extends AbstractSSTableIterator
// Return what should be returned at the end of this, or null if nothing should.
private Unfiltered handlePreSliceData() throws IOException
{
+ assert deserializer != null;
+
// Note that the following comparison is not strict. The reason is that the only cases
// where it can be == is if the "next" is a RT start marker (either a '[' of a ')[' boundary),
// and if we had a strict inequality and an open RT marker before this, we would issue
@@ -126,6 +121,8 @@ public class SSTableIterator extends AbstractSSTableIterator
// if we're done with the slice.
protected Unfiltered computeNext() throws IOException
{
+ assert deserializer != null;
+
if (!deserializer.hasNext() || deserializer.compareNextTo(end) > 0)
return null;
@@ -143,8 +140,6 @@ public class SSTableIterator extends AbstractSSTableIterator
if (sliceDone)
return false;
- assert deserializer != null;
-
if (start != null)
{
Unfiltered unfiltered = handlePreSliceData();
@@ -187,28 +182,18 @@ public class SSTableIterator extends AbstractSSTableIterator
private int lastBlockIdx; // the last index block that has data for the current query
- private ForwardIndexedReader(RowIndexEntry indexEntry, FileDataInput file, boolean isAtPartitionStart, boolean shouldCloseFile)
+ private ForwardIndexedReader(RowIndexEntry indexEntry, FileDataInput file, boolean shouldCloseFile)
{
- super(file, isAtPartitionStart, shouldCloseFile);
+ super(file, shouldCloseFile);
this.indexState = new IndexState(this, sstable.metadata.comparator, indexEntry, false);
this.lastBlockIdx = indexState.blocksCount(); // if we never call setForSlice, that's where we want to stop
}
@Override
- protected void init() throws IOException
- {
- // If this is called, it means we're calling hasNext() before any call to setForSlice. Which means
- // we're reading everything from the beginning. So just set us up at the beginning of the first block.
- indexState.setToBlock(0);
- }
-
- @Override
public void setForSlice(Slice slice) throws IOException
{
super.setForSlice(slice);
- isInit = true;
-
// if our previous slicing already got us the biggest row in the sstable, we're done
if (indexState.isDone())
{
@@ -265,6 +250,7 @@ public class SSTableIterator extends AbstractSSTableIterator
protected Unfiltered computeNext() throws IOException
{
// Our previous read might have made us cross an index block boundary. If so, update our informations.
+ // If we read from the beginning of the partition, this is also what will initialize the index state.
indexState.updateBlock();
// Return the next unfiltered unless we've reached the end, or we're beyond our slice
http://git-wip-us.apache.org/repos/asf/cassandra/blob/bc887acf/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 66c32ee..14cec36 100644
--- a/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java
+++ b/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java
@@ -49,11 +49,11 @@ public class SSTableReversedIterator extends AbstractSSTableIterator
super(sstable, file, key, indexEntry, columns, isForThrift);
}
- protected Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean isAtPartitionStart, boolean shouldCloseFile)
+ protected Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean shouldCloseFile)
{
return indexEntry.isIndexed()
- ? new ReverseIndexedReader(indexEntry, file, isAtPartitionStart, shouldCloseFile)
- : new ReverseReader(file, isAtPartitionStart, shouldCloseFile);
+ ? new ReverseIndexedReader(indexEntry, file, shouldCloseFile)
+ : new ReverseReader(file, shouldCloseFile);
}
public boolean isReverseOrder()
@@ -66,9 +66,9 @@ public class SSTableReversedIterator extends AbstractSSTableIterator
protected ReusablePartitionData buffer;
protected Iterator<Unfiltered> iterator;
- private ReverseReader(FileDataInput file, boolean isAtPartitionStart, boolean shouldCloseFile)
+ private ReverseReader(FileDataInput file, boolean shouldCloseFile)
{
- super(file, isAtPartitionStart, shouldCloseFile);
+ super(file, shouldCloseFile);
}
protected ReusablePartitionData createBuffer(int blocksCount)
@@ -100,13 +100,6 @@ public class SSTableReversedIterator extends AbstractSSTableIterator
return new ReusablePartitionData(metadata(), partitionKey(), columns(), estimatedRowCount);
}
- protected void init() throws IOException
- {
- // We should always have been initialized (at the beginning of the partition). Only indexed readers may
- // have to initialize.
- throw new IllegalStateException();
- }
-
public void setForSlice(Slice slice) throws IOException
{
// If we have read the data, just create the iterator for the slice. Otherwise, read the data.
@@ -212,23 +205,16 @@ public class SSTableReversedIterator extends AbstractSSTableIterator
// The last index block to consider for the slice
private int lastBlockIdx;
- private ReverseIndexedReader(RowIndexEntry indexEntry, FileDataInput file, boolean isAtPartitionStart, boolean shouldCloseFile)
+ private ReverseIndexedReader(RowIndexEntry indexEntry, FileDataInput file, boolean shouldCloseFile)
{
- super(file, isAtPartitionStart, shouldCloseFile);
+ super(file, shouldCloseFile);
this.indexState = new IndexState(this, sstable.metadata.comparator, indexEntry, true);
}
- protected void init() throws IOException
- {
- // This is actually a no-op, because if we call hasNext without having called setForSlice, then ReverseReader.hasNextInternal
- // will call setForSlice(Slice.ALL) which does the right thing.
- }
-
@Override
public void setForSlice(Slice slice) throws IOException
{
this.slice = slice;
- isInit = true;
// if our previous slicing already got us past the beginning of the sstable, we're done
if (indexState.isDone())
http://git-wip-us.apache.org/repos/asf/cassandra/blob/bc887acf/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java b/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
new file mode 100644
index 0000000..00f0ca2
--- /dev/null
+++ b/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
@@ -0,0 +1,80 @@
+/*
+ * 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.cql3;
+
+import java.util.Random;
+
+import org.junit.Test;
+
+import org.apache.cassandra.Util;
+import org.apache.cassandra.db.DecoratedKey;
+import org.apache.cassandra.db.RowIndexEntry;
+import org.apache.cassandra.io.sstable.format.SSTableReader;
+import org.apache.cassandra.utils.ByteBufferUtil;
+
+public class QueryWithIndexedSSTableTest extends CQLTester
+{
+ @Test
+ public void queryIndexedSSTableTest() throws Throwable
+ {
+ // That test reproduces the bug from CASSANDRA-10903 and the fact we have a static column is
+ // relevant to that reproduction in particular as it forces a slightly different code path that
+ // if there wasn't a static.
+
+ int ROWS = 1000;
+ int VALUE_LENGTH = 100;
+
+ createTable("CREATE TABLE %s (k int, t int, s text static, v text, PRIMARY KEY (k, t))");
+
+ // We create a partition that is big enough that the underlying sstable will be indexed
+ // For that, we use a large-ish number of row, and a value that isn't too small.
+ String text = makeRandomTest(VALUE_LENGTH);
+ for (int i = 0; i < ROWS; i++)
+ execute("INSERT INTO %s(k, t, v) VALUES (?, ?, ?)", 0, i, text + i);
+
+ flush();
+ compact();
+
+ // Sanity check that we're testing what we want to test, that is that we're reading from an indexed
+ // sstable. Note that we'll almost surely have a single indexed sstable in practice, but it's theorically
+ // possible for a compact strategy to yield more than that and as long as one is indexed we're pretty
+ // much testing what we want. If this check ever fails on some specific setting, we'll have to either
+ // tweak ROWS and VALUE_LENGTH, or skip the test on those settings.
+ DecoratedKey dk = Util.dk(ByteBufferUtil.bytes(0));
+ boolean hasIndexed = false;
+ for (SSTableReader sstable : getCurrentColumnFamilyStore().getLiveSSTables())
+ {
+ RowIndexEntry indexEntry = sstable.getPosition(dk, SSTableReader.Operator.EQ);
+ hasIndexed |= indexEntry != null && indexEntry.isIndexed();
+ }
+ assert hasIndexed;
+
+ assertRowCount(execute("SELECT s FROM %s WHERE k = ?", 0), ROWS);
+ }
+
+ // Creates a random string
+ public static String makeRandomTest(int length)
+ {
+ Random random = new Random();
+ char[] chars = new char[26];
+ int i = 0;
+ for (char c = 'a'; c <= 'z'; c++)
+ chars[i++] = c;
+ return new String(chars);
+ }
+}
[2/2] cassandra git commit: Assert !reversed in updateBlock and
improve test
Posted by sl...@apache.org.
Assert !reversed in updateBlock and improve test
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/e7c6e14f
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/e7c6e14f
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/e7c6e14f
Branch: refs/heads/10903
Commit: e7c6e14fd2662c35310d1b27017aed75640ce002
Parents: bc887ac
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Wed Dec 23 12:13:54 2015 +0100
Committer: Sylvain Lebresne <sy...@datastax.com>
Committed: Wed Dec 23 12:18:38 2015 +0100
----------------------------------------------------------------------
.../cassandra/db/columniterator/AbstractSSTableIterator.java | 4 ++++
.../org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java | 4 ++++
2 files changed, 8 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cassandra/blob/e7c6e14f/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java b/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
index f103ee2..8ac3dcb 100644
--- a/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
+++ b/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
@@ -443,8 +443,12 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
}
// Update the block idx based on the current reader position if we're past the current block.
+ // This only makes sense for forward iteration (for reverse ones, when we reach the end of a block we
+ // should seek to the previous one, not update the index state and continue).
public void updateBlock() throws IOException
{
+ assert !reversed;
+
// If we get here with currentBlockIdx < 0, it means setToBlock() has never been called, so it means
// we're about to read from the beginning of the partition, but haven't "prepared" the IndexState yet.
// Do so by setting us on the first block.
http://git-wip-us.apache.org/repos/asf/cassandra/blob/e7c6e14f/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java b/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
index 00f0ca2..4838392 100644
--- a/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
+++ b/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
@@ -65,6 +65,10 @@ public class QueryWithIndexedSSTableTest extends CQLTester
assert hasIndexed;
assertRowCount(execute("SELECT s FROM %s WHERE k = ?", 0), ROWS);
+ assertRowCount(execute("SELECT s FROM %s WHERE k = ? ORDER BY t DESC", 0), ROWS);
+
+ assertRowCount(execute("SELECT DISTINCT s FROM %s WHERE k = ?", 0), 1);
+ assertRowCount(execute("SELECT DISTINCT s FROM %s WHERE k = ? ORDER BY t DESC", 0), 1);
}
// Creates a random string