Repository: cassandra Updated Branches: refs/heads/trunk e71a49e81 -> 3580f6c05
Fix secondary index queries regression patch by Benjamin Lerer; reviewed by Sylvain Lebresne for CASSANDRA-13013 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/77f0f683 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/77f0f683 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/77f0f683 Branch: refs/heads/trunk Commit: 77f0f68313a4c36bf86363e71f4775e36ccf85bc Parents: 078a841 Author: Benjamin Lerer <b.le...@gmail.com> Authored: Fri Jan 27 16:06:25 2017 +0100 Committer: Benjamin Lerer <b.le...@gmail.com> Committed: Fri Jan 27 16:06:25 2017 +0100 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../cql3/restrictions/RestrictionSet.java | 23 ++++++++-- .../restrictions/StatementRestrictions.java | 26 +++++++++--- .../cql3/statements/ModificationStatement.java | 8 ++-- .../cql3/statements/SelectStatement.java | 13 +++++- .../validation/entities/SecondaryIndexTest.java | 44 ++++++++++++++++++++ 6 files changed, 100 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/77f0f683/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 6f7b5c2..a32ef2f 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.10 + * Fix secondary index queries regression (CASSANDRA-13013) * Add duration type to the protocol V5 (CASSANDRA-12850) * Fix duration type validation (CASSANDRA-13143) * Fix flaky GcCompactionTest (CASSANDRA-12664) http://git-wip-us.apache.org/repos/asf/cassandra/blob/77f0f683/src/java/org/apache/cassandra/cql3/restrictions/RestrictionSet.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/restrictions/RestrictionSet.java b/src/java/org/apache/cassandra/cql3/restrictions/RestrictionSet.java index 0876f3e..3a1bcb1 100644 --- a/src/java/org/apache/cassandra/cql3/restrictions/RestrictionSet.java +++ b/src/java/org/apache/cassandra/cql3/restrictions/RestrictionSet.java @@ -72,14 +72,14 @@ final class RestrictionSet implements Restrictions, Iterable<SingleRestriction> } @Override - public final void addRowFilterTo(RowFilter filter, SecondaryIndexManager indexManager, QueryOptions options) throws InvalidRequestException + public void addRowFilterTo(RowFilter filter, SecondaryIndexManager indexManager, QueryOptions options) throws InvalidRequestException { for (Restriction restriction : restrictions.values()) restriction.addRowFilterTo(filter, indexManager, options); } @Override - public final List<ColumnDefinition> getColumnDefs() + public List<ColumnDefinition> getColumnDefs() { return new ArrayList<>(restrictions.keySet()); } @@ -92,18 +92,33 @@ final class RestrictionSet implements Restrictions, Iterable<SingleRestriction> } @Override - public final boolean isEmpty() + public boolean isEmpty() { return restrictions.isEmpty(); } @Override - public final int size() + public int size() { return restrictions.size(); } /** + * Checks if one of the restrictions applies to a column of the specific kind. + * @param kind the column kind + * @return {@code true} if one of the restrictions applies to a column of the specific kind, {@code false} otherwise. + */ + public boolean hasRestrictionFor(ColumnDefinition.Kind kind) + { + for (ColumnDefinition column : restrictions.keySet()) + { + if (column.kind == kind) + return true; + } + return false; + } + + /** * Adds the specified restriction to this set of restrictions. * * @param restriction the restriction to add http://git-wip-us.apache.org/repos/asf/cassandra/blob/77f0f683/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java b/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java index 6b89579..8a8ee56 100644 --- a/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java +++ b/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java @@ -24,6 +24,7 @@ import com.google.common.base.Joiner; import org.apache.cassandra.config.CFMetaData; import org.apache.cassandra.config.ColumnDefinition; +import org.apache.cassandra.config.ColumnDefinition.Kind; import org.apache.cassandra.cql3.*; import org.apache.cassandra.cql3.functions.Function; import org.apache.cassandra.cql3.statements.Bound; @@ -96,6 +97,12 @@ public final class StatementRestrictions private boolean isKeyRange; /** + * <code>true</code> if nonPrimaryKeyRestrictions contains restriction on a regular column, + * <code>false</code> otherwise. + */ + private boolean hasRegularColumnsRestrictions; + + /** * Creates a new empty <code>StatementRestrictions</code>. * * @param type the type of statement @@ -128,7 +135,6 @@ public final class StatementRestrictions { this(type, cfm, allowFiltering); - ColumnFamilyStore cfs; SecondaryIndexManager secondaryIndexManager = null; @@ -174,6 +180,8 @@ public final class StatementRestrictions } } + hasRegularColumnsRestrictions = nonPrimaryKeyRestrictions.hasRestrictionFor(Kind.REGULAR); + boolean hasQueriableClusteringColumnIndex = false; boolean hasQueriableIndex = false; @@ -197,7 +205,7 @@ public final class StatementRestrictions if (usesSecondaryIndexing || partitionKeyRestrictions.needFiltering(cfm)) filterRestrictions.add(partitionKeyRestrictions); - if (selectsOnlyStaticColumns && hasClusteringColumnsRestriction()) + if (selectsOnlyStaticColumns && hasClusteringColumnsRestrictions()) { // If the only updated/deleted columns are static, then we don't need clustering columns. // And in fact, unless it is an INSERT, we reject if clustering colums are provided as that @@ -499,7 +507,7 @@ public final class StatementRestrictions "Slice restrictions are not supported on the clustering columns in %s statements", type); if (!type.allowClusteringColumnSlices() - && (!cfm.isCompactTable() || (cfm.isCompactTable() && !hasClusteringColumnsRestriction()))) + && (!cfm.isCompactTable() || (cfm.isCompactTable() && !hasClusteringColumnsRestrictions()))) { if (!selectsOnlyStaticColumns && hasUnrestrictedClusteringColumns()) throw invalidRequest("Some clustering keys are missing: %s", @@ -513,7 +521,7 @@ public final class StatementRestrictions "Clustering columns can only be restricted with CONTAINS with a secondary index or filtering"); - if (hasClusteringColumnsRestriction() && clusteringColumnsRestrictions.needFiltering()) + if (hasClusteringColumnsRestrictions() && clusteringColumnsRestrictions.needFiltering()) { if (hasQueriableIndex || forView) { @@ -732,7 +740,7 @@ public final class StatementRestrictions * @return <code>true</code> if the query has some restrictions on the clustering columns, * <code>false</code> otherwise. */ - public boolean hasClusteringColumnsRestriction() + public boolean hasClusteringColumnsRestrictions() { return !clusteringColumnsRestrictions.isEmpty(); } @@ -824,4 +832,12 @@ public final class StatementRestrictions && (clusteringColumnsRestrictions.hasOnlyEqualityRestrictions()); } + /** + * Checks if one of the restrictions applies to a regular column. + * @return {@code true} if one of the restrictions applies to a regular column, {@code false} otherwise. + */ + public boolean hasRegularColumnsRestrictions() + { + return hasRegularColumnsRestrictions; + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/77f0f683/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java b/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java index 5f3a2b3..08bb6ba 100644 --- a/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java @@ -254,7 +254,7 @@ public abstract class ModificationStatement implements CQLStatement // columns is if we set some static columns, and in that case no clustering // columns should be given. So in practice, it's enough to check if we have // either the table has no clustering or if it has at least one of them set. - return cfm.clusteringColumns().isEmpty() || restrictions.hasClusteringColumnsRestriction(); + return cfm.clusteringColumns().isEmpty() || restrictions.hasClusteringColumnsRestrictions(); } public boolean updatesStaticRow() @@ -305,7 +305,7 @@ public abstract class ModificationStatement implements CQLStatement public NavigableSet<Clustering> createClustering(QueryOptions options) throws InvalidRequestException { - if (appliesOnlyToStaticColumns() && !restrictions.hasClusteringColumnsRestriction()) + if (appliesOnlyToStaticColumns() && !restrictions.hasClusteringColumnsRestrictions()) return FBUtilities.singleton(CBuilder.STATIC_BUILDER.build(), cfm.comparator); return restrictions.getClusteringColumns(options); @@ -630,7 +630,7 @@ public abstract class ModificationStatement implements CQLStatement List<ByteBuffer> keys = buildPartitionKeyNames(options); if (type.allowClusteringColumnSlices() - && restrictions.hasClusteringColumnsRestriction() + && restrictions.hasClusteringColumnsRestrictions() && restrictions.isColumnRange()) { Slices slices = createSlice(options); @@ -670,7 +670,7 @@ public abstract class ModificationStatement implements CQLStatement PartitionUpdate upd = collector.getPartitionUpdate(cfm, dk, options.getConsistency()); - if (!restrictions.hasClusteringColumnsRestriction()) + if (!restrictions.hasClusteringColumnsRestrictions()) { addUpdateForKey(upd, Clustering.EMPTY, params); } http://git-wip-us.apache.org/repos/asf/cassandra/blob/77f0f683/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java index 038d4bd..e8b4600 100644 --- a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java @@ -796,7 +796,7 @@ public class SelectStatement implements CQLStatement // we want to include static columns and we're done. if (!partition.hasNext()) { - if (!staticRow.isEmpty() && (!restrictions.hasClusteringColumnsRestriction() || cfm.isStaticCompactTable())) + if (!staticRow.isEmpty() && (queriesFullPartitions() || cfm.isStaticCompactTable())) { result.newRow(partition.partitionKey(), staticRow.clustering()); for (ColumnDefinition def : selection.getColumns()) @@ -843,6 +843,15 @@ public class SelectStatement implements CQLStatement } } + /** + * Checks if the query is a full partitions selection. + * @return {@code true} if the query is a full partitions selection, {@code false} otherwise. + */ + private boolean queriesFullPartitions() + { + return !restrictions.hasClusteringColumnsRestrictions() && !restrictions.hasRegularColumnsRestrictions(); + } + private static void addValue(Selection.ResultSetBuilder result, ColumnDefinition def, Row row, int nowInSec, ProtocolVersion protocolVersion) { if (def.isComplex()) @@ -1007,7 +1016,7 @@ public class SelectStatement implements CQLStatement StatementRestrictions restrictions) throws InvalidRequestException { - checkFalse(restrictions.hasClusteringColumnsRestriction() || + checkFalse(restrictions.hasClusteringColumnsRestrictions() || (restrictions.hasNonPrimaryKeyRestrictions() && !restrictions.nonPKRestrictedColumns(true).stream().allMatch(ColumnDefinition::isStatic)), "SELECT DISTINCT with WHERE clause only supports restriction by partition key and/or static columns."); http://git-wip-us.apache.org/repos/asf/cassandra/blob/77f0f683/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java index 5cb76c9..8a8bdcc 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java @@ -1299,6 +1299,50 @@ public class SecondaryIndexTest extends CQLTester }); } + @Test + public void testIndexOnStaticColumnWithPartitionWithoutRows() throws Throwable + { + createTable("CREATE TABLE %s (pk int, c int, s int static, v int, PRIMARY KEY(pk, c))"); + createIndex("CREATE INDEX ON %s (s)"); + + execute("INSERT INTO %s (pk, c, s, v) VALUES (?, ?, ?, ?)", 1, 1, 9, 1); + execute("INSERT INTO %s (pk, c, s, v) VALUES (?, ?, ?, ?)", 1, 2, 9, 2); + execute("INSERT INTO %s (pk, s) VALUES (?, ?)", 2, 9); + execute("INSERT INTO %s (pk, c, s, v) VALUES (?, ?, ?, ?)", 3, 1, 9, 1); + flush(); + + assertRows(execute("SELECT * FROM %s WHERE s = ?", 9), + row(1, 1, 9, 1), + row(1, 2, 9, 2), + row(2, null, 9, null), + row(3, 1, 9, 1)); + + execute("DELETE FROM %s WHERE pk = ?", 3); + + assertRows(execute("SELECT * FROM %s WHERE s = ?", 9), + row(1, 1, 9, 1), + row(1, 2, 9, 2), + row(2, null, 9, null)); + } + + @Test + public void testIndexOnRegularColumnWithPartitionWithoutRows() throws Throwable + { + createTable("CREATE TABLE %s (pk int, c int, s int static, v int, PRIMARY KEY(pk, c))"); + createIndex("CREATE INDEX ON %s (v)"); + + execute("INSERT INTO %s (pk, c, s, v) VALUES (?, ?, ?, ?)", 1, 1, 9, 1); + execute("INSERT INTO %s (pk, c, s, v) VALUES (?, ?, ?, ?)", 1, 2, 9, 2); + execute("INSERT INTO %s (pk, s) VALUES (?, ?)", 2, 9); + execute("INSERT INTO %s (pk, c, s, v) VALUES (?, ?, ?, ?)", 3, 1, 9, 1); + flush(); + + execute("DELETE FROM %s WHERE pk = ? and c = ?", 3, 1); + + assertRows(execute("SELECT * FROM %s WHERE v = ?", 1), + row(1, 1, 9, 1)); + } + private ResultMessage.Prepared prepareStatement(String cql, boolean forThrift) { return QueryProcessor.prepare(String.format(cql, KEYSPACE, currentTable()),
