From 979e5590e0c2e7c33e5ffb6e56c67d855fc17813 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Tue, 19 Jul 2016 15:07:37 +0200 Subject: [PATCH] NullPointerExpception when reading/compacting table patch by Sylvain Lebresne; reviewed by Carl Yeksigian for CASSANDRA-11988 --- CHANGES.txt | 1 + .../db/partitions/AbstractBTreePartition.java | 2 +- .../db/partitions/PurgeFunction.java | 15 ++++++++----- .../cassandra/db/rows/BaseRowIterator.java | 2 +- .../org/apache/cassandra/db/rows/Row.java | 13 ++++++++--- .../cassandra/db/transform/BaseRows.java | 2 +- .../apache/cassandra/db/transform/Filter.java | 12 ++++++---- .../entities/StaticColumnsTest.java | 22 +++++++++++++++++++ 8 files changed, 54 insertions(+), 15 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 739c095739..6ea39cd8f2 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.0.9 + * NullPointerExpception when reading/compacting table (CASSANDRA-11988) * Fix problem with undeleteable rows on upgrade to new sstable format (CASSANDRA-12144) * Fix paging logic for deleted partitions with static columns (CASSANDRA-12107) * Wait until the message is being send to decide which serializer must be used (CASSANDRA-11393) diff --git a/src/java/org/apache/cassandra/db/partitions/AbstractBTreePartition.java b/src/java/org/apache/cassandra/db/partitions/AbstractBTreePartition.java index b7fa691746..1fa3324c69 100644 --- a/src/java/org/apache/cassandra/db/partitions/AbstractBTreePartition.java +++ b/src/java/org/apache/cassandra/db/partitions/AbstractBTreePartition.java @@ -60,7 +60,7 @@ public abstract class AbstractBTreePartition implements Partition, Iterable this.columns = columns; this.tree = tree; this.deletionInfo = deletionInfo; - this.staticRow = staticRow; + this.staticRow = staticRow == null ? Rows.EMPTY_STATIC_ROW : staticRow; this.stats = stats; } } diff --git a/src/java/org/apache/cassandra/db/partitions/PurgeFunction.java b/src/java/org/apache/cassandra/db/partitions/PurgeFunction.java index 492bab1c7d..d3255d303f 100644 --- a/src/java/org/apache/cassandra/db/partitions/PurgeFunction.java +++ b/src/java/org/apache/cassandra/db/partitions/PurgeFunction.java @@ -55,7 +55,8 @@ public abstract class PurgeFunction extends Transformation extends CloseableIterator /** * The static part corresponding to this partition (this can be an empty - * row). + * row but cannot be {@code null}). */ public Row staticRow(); diff --git a/src/java/org/apache/cassandra/db/rows/Row.java b/src/java/org/apache/cassandra/db/rows/Row.java index 82c07a75c9..c7c321696d 100644 --- a/src/java/org/apache/cassandra/db/rows/Row.java +++ b/src/java/org/apache/cassandra/db/rows/Row.java @@ -200,7 +200,8 @@ public interface Row extends Unfiltered, Collection * * @param purger the {@code DeletionPurger} to use to decide what can be purged. * @param nowInSec the current time to decide what is deleted and what isn't (in the case of expired cells). - * @return this row but without any deletion info purged by {@code purger}. + * @return this row but without any deletion info purged by {@code purger}. If the purged row is empty, returns + * {@code null}. */ public Row purge(DeletionPurger purger, int nowInSec); @@ -210,8 +211,14 @@ public interface Row extends Unfiltered, Collection public Row markCounterLocalToBeCleared(); /** - * returns a copy of this row where all live timestamp have been replaced by {@code newTimestamp} and every deletion timestamp - * by {@code newTimestamp - 1}. See {@link Commit} for why we need this. + * Returns a copy of this row where all live timestamp have been replaced by {@code newTimestamp} and every deletion + * timestamp by {@code newTimestamp - 1}. + * + * @param newTimestamp the timestamp to use for all live data in the returned row. + * @param a copy of this row with timestamp updated using {@code newTimestamp}. This can return {@code null} in the + * rare where the row only as a shadowable row deletion and the new timestamp supersedes it. + * + * @see Commit for why we need this. */ public Row updateAllTimestamp(long newTimestamp); diff --git a/src/java/org/apache/cassandra/db/transform/BaseRows.java b/src/java/org/apache/cassandra/db/transform/BaseRows.java index b0e642b724..7b0bb99d97 100644 --- a/src/java/org/apache/cassandra/db/transform/BaseRows.java +++ b/src/java/org/apache/cassandra/db/transform/BaseRows.java @@ -69,7 +69,7 @@ implements BaseRowIterator public Row staticRow() { - return staticRow; + return staticRow == null ? Rows.EMPTY_STATIC_ROW : staticRow; } diff --git a/src/java/org/apache/cassandra/db/transform/Filter.java b/src/java/org/apache/cassandra/db/transform/Filter.java index 138d3c8dbc..48c8b1a4d9 100644 --- a/src/java/org/apache/cassandra/db/transform/Filter.java +++ b/src/java/org/apache/cassandra/db/transform/Filter.java @@ -33,7 +33,8 @@ final class Filter extends Transformation this.nowInSec = nowInSec; } - public RowIterator applyToPartition(BaseRowIterator iterator) + @Override + protected RowIterator applyToPartition(BaseRowIterator iterator) { RowIterator filtered = iterator instanceof UnfilteredRows ? new FilteredRows(this, (UnfilteredRows) iterator) @@ -45,7 +46,8 @@ final class Filter extends Transformation return filtered; } - public Row applyToStatic(Row row) + @Override + protected Row applyToStatic(Row row) { if (row.isEmpty()) return Rows.EMPTY_STATIC_ROW; @@ -54,12 +56,14 @@ final class Filter extends Transformation return row == null ? Rows.EMPTY_STATIC_ROW : row; } - public Row applyToRow(Row row) + @Override + protected Row applyToRow(Row row) { return row.purge(DeletionPurger.PURGE_ALL, nowInSec); } - public RangeTombstoneMarker applyToMarker(RangeTombstoneMarker marker) + @Override + protected RangeTombstoneMarker applyToMarker(RangeTombstoneMarker marker) { return null; } diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/StaticColumnsTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/StaticColumnsTest.java index cef6f1f326..75cbcc738d 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/StaticColumnsTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/StaticColumnsTest.java @@ -268,4 +268,26 @@ public class StaticColumnsTest extends CQLTester // We shouldn 't allow static when there is not clustering columns assertInvalid("ALTER TABLE %s ADD bar2 text static"); } + + /** + * Ensure that deleting and compacting a static row that should be purged doesn't throw. + * This is a test for #11988. + */ + @Test + public void testStaticColumnPurging() throws Throwable + { + createTable("CREATE TABLE %s (pkey text, ckey text, value text, static_value text static, PRIMARY KEY(pkey, ckey)) WITH gc_grace_seconds = 0"); + + execute("INSERT INTO %s (pkey, ckey, static_value, value) VALUES (?, ?, ?, ?)", "k1", "c1", "s1", "v1"); + + flush(); + + execute("DELETE static_value FROM %s WHERE pkey = ?", "k1"); + + flush(); + + compact(); + + assertRows(execute("SELECT * FROM %s"), row("k1", "c1", null, "v1")); + } }