Consider secondary indexes when preparing clustering column restrictions Patch by Sam Tunnicliffe; reviewed by Benjamin Lerer for CASSANDRA-11510
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/dff7b447 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/dff7b447 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/dff7b447 Branch: refs/heads/cassandra-3.0 Commit: dff7b447a74d639888eca76c940c057d3fa647e7 Parents: 38e973d Author: Sam Tunnicliffe <[email protected]> Authored: Wed Apr 20 12:01:53 2016 +0100 Committer: Sam Tunnicliffe <[email protected]> Committed: Fri Apr 29 11:46:16 2016 +0100 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../restrictions/PrimaryKeyRestrictionSet.java | 71 +++++++++++++++++++- .../restrictions/StatementRestrictions.java | 27 +++++--- .../SelectSingleColumnRelationTest.java | 6 +- 4 files changed, 90 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/dff7b447/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index d5cccfa..fb06cd6 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 2.2.7 + * Restore ability to filter on clustering columns when using a 2i (CASSANDRA-11510) * JSON datetime formatting needs timezone (CASSANDRA-11137) * Fix is_dense recalculation for Thrift-updated tables (CASSANDRA-11502) * Remove unnescessary file existence check during anticompaction (CASSANDRA-11660) http://git-wip-us.apache.org/repos/asf/cassandra/blob/dff7b447/src/java/org/apache/cassandra/cql3/restrictions/PrimaryKeyRestrictionSet.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/restrictions/PrimaryKeyRestrictionSet.java b/src/java/org/apache/cassandra/cql3/restrictions/PrimaryKeyRestrictionSet.java index 7ce228c..0c10f13 100644 --- a/src/java/org/apache/cassandra/cql3/restrictions/PrimaryKeyRestrictionSet.java +++ b/src/java/org/apache/cassandra/cql3/restrictions/PrimaryKeyRestrictionSet.java @@ -22,16 +22,18 @@ import java.util.*; import com.google.common.collect.Lists; -import org.apache.cassandra.db.composites.Composite; +import org.apache.cassandra.config.CFMetaData; import org.apache.cassandra.config.ColumnDefinition; import org.apache.cassandra.cql3.QueryOptions; import org.apache.cassandra.cql3.functions.Function; import org.apache.cassandra.cql3.statements.Bound; import org.apache.cassandra.db.IndexExpression; +import org.apache.cassandra.db.Keyspace; import org.apache.cassandra.db.composites.*; import org.apache.cassandra.db.composites.Composite.EOC; import org.apache.cassandra.db.index.SecondaryIndexManager; import org.apache.cassandra.exceptions.InvalidRequestException; + import static org.apache.cassandra.cql3.statements.RequestValidations.checkFalse; import static org.apache.cassandra.cql3.statements.RequestValidations.invalidRequest; @@ -65,20 +67,48 @@ final class PrimaryKeyRestrictionSet extends AbstractPrimaryKeyRestrictions */ private boolean contains; + + /** + * If restrictions apply to clustering columns, we need to check whether they can be satisfied by an index lookup + * as this affects which other restrictions can legally be specified (if an index is present, we are more lenient + * about what additional filtering can be performed on the results of a lookup - see CASSANDRA-11510). + * + * We don't hold a reference to the SecondaryIndexManager itself as this is not strictly a singleton (although + * we often treat is as one), the field would also require annotation with @Unmetered to avoid blowing up the + * object size (used when calculating the size of prepared statements for caching). Instead, we refer to the + * CFMetaData and retrieve the index manager when necessary. + * + * There are a couple of scenarios where the CFM can be null (and we make sure and test for null when we use it): + * * where an empty set of restrictions are created for use in processing query results - see + * SelectStatement.forSelection + * * where the restrictions apply to partition keys and not clustering columns e.g. + * StatementRestrictions.partitionKeyRestrictions + * * in unit tests (in particular PrimaryKeyRestrictionSetTest which is primarily concerned with the correct + * generation of bounds when secondary indexes are not used). + */ + private final CFMetaData cfm; + public PrimaryKeyRestrictionSet(CType ctype) { + this(ctype, null); + } + + public PrimaryKeyRestrictionSet(CType ctype, CFMetaData cfm) + { super(ctype); + this.cfm = cfm; this.restrictions = new RestrictionSet(); this.eq = true; } private PrimaryKeyRestrictionSet(PrimaryKeyRestrictionSet primaryKeyRestrictions, - Restriction restriction) throws InvalidRequestException + Restriction restriction) throws InvalidRequestException { super(primaryKeyRestrictions.ctype); this.restrictions = primaryKeyRestrictions.restrictions.addRestriction(restriction); + this.cfm = primaryKeyRestrictions.cfm; - if (!primaryKeyRestrictions.isEmpty()) + if (!primaryKeyRestrictions.isEmpty() && !hasSupportingIndex(restriction)) { ColumnDefinition lastRestrictionStart = primaryKeyRestrictions.restrictions.lastRestriction().getFirstColumn(); ColumnDefinition newRestrictionStart = restriction.getFirstColumn(); @@ -104,6 +134,12 @@ final class PrimaryKeyRestrictionSet extends AbstractPrimaryKeyRestrictions this.eq = true; } + private boolean hasSupportingIndex(Restriction restriction) + { + return cfm != null + && restriction.hasSupportingIndex(Keyspace.open(cfm.ksName).getColumnFamilyStore(cfm.cfId).indexManager); + } + @Override public boolean isSlice() { @@ -392,4 +428,33 @@ final class PrimaryKeyRestrictionSet extends AbstractPrimaryKeyRestrictions { return restrictions.lastColumn(); } + + public final boolean needsFiltering() + { + // Backported from ClusteringColumnRestrictions from CASSANDRA-11310 for 3.6 + // As that suggests, this should only be called on clustering column + // and not partition key restrictions. + int position = 0; + Restriction slice = null; + for (Restriction restriction : restrictions) + { + if (handleInFilter(restriction, position)) + return true; + + if (slice != null && !slice.getFirstColumn().equals(restriction.getFirstColumn())) + return true; + + if (slice == null && restriction.isSlice()) + slice = restriction; + else + position = restriction.getLastColumn().position() + 1; + } + + return false; + } + + private boolean handleInFilter(Restriction restriction, int index) + { + return restriction.isContains() || index != restriction.getFirstColumn().position(); + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/dff7b447/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 3934f33..5b7c58d 100644 --- a/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java +++ b/src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java @@ -25,6 +25,7 @@ import com.google.common.collect.Iterables; import org.apache.cassandra.config.CFMetaData; import org.apache.cassandra.config.ColumnDefinition; +import org.apache.cassandra.config.Schema; import org.apache.cassandra.cql3.*; import org.apache.cassandra.cql3.functions.Function; import org.apache.cassandra.cql3.statements.Bound; @@ -115,7 +116,7 @@ public final class StatementRestrictions { this.cfm = cfm; this.partitionKeyRestrictions = new PrimaryKeyRestrictionSet(cfm.getKeyValidatorAsCType()); - this.clusteringColumnsRestrictions = new PrimaryKeyRestrictionSet(cfm.comparator); + this.clusteringColumnsRestrictions = new PrimaryKeyRestrictionSet(cfm.comparator, cfm); this.nonPrimaryKeyRestrictions = new RestrictionSet(); /* @@ -128,9 +129,7 @@ public final class StatementRestrictions for (Relation relation : whereClause) addRestriction(relation.toRestriction(cfm, boundNames)); - ColumnFamilyStore cfs = Keyspace.open(cfm.ksName).getColumnFamilyStore(cfm.cfName); - SecondaryIndexManager secondaryIndexManager = cfs.indexManager; - + SecondaryIndexManager secondaryIndexManager = Keyspace.open(cfm.ksName).getColumnFamilyStore(cfm.cfName).indexManager; boolean hasQueriableClusteringColumnIndex = clusteringColumnsRestrictions.hasSupportingIndex(secondaryIndexManager); boolean hasQueriableIndex = hasQueriableClusteringColumnIndex || partitionKeyRestrictions.hasSupportingIndex(secondaryIndexManager) @@ -313,8 +312,14 @@ public final class StatementRestrictions checkFalse(clusteringColumnsRestrictions.isContains() && !hasQueriableIndex, "Cannot restrict clustering columns by a CONTAINS relation without a secondary index"); - if (hasClusteringColumnsRestriction()) + if (hasClusteringColumnsRestriction() && clusteringRestrictionsNeedFiltering()) { + if (hasQueriableIndex) + { + usesSecondaryIndexing = true; + return; + } + List<ColumnDefinition> clusteringColumns = cfm.clusteringColumns(); List<ColumnDefinition> restrictedColumns = new LinkedList<>(clusteringColumnsRestrictions.getColumnDefs()); @@ -325,19 +330,19 @@ public final class StatementRestrictions if (!clusteringColumn.equals(restrictedColumn)) { - checkTrue(hasQueriableIndex, + throw invalidRequest( "PRIMARY KEY column \"%s\" cannot be restricted as preceding column \"%s\" is not restricted", restrictedColumn.name, clusteringColumn.name); - - usesSecondaryIndexing = true; // handle gaps and non-keyrange cases. - break; } } } + } - if (clusteringColumnsRestrictions.isContains()) - usesSecondaryIndexing = true; + public final boolean clusteringRestrictionsNeedFiltering() + { + assert clusteringColumnsRestrictions instanceof PrimaryKeyRestrictionSet; + return ((PrimaryKeyRestrictionSet) clusteringColumnsRestrictions).needsFiltering(); } public List<IndexExpression> getIndexExpressions(SecondaryIndexManager indexManager, http://git-wip-us.apache.org/repos/asf/cassandra/blob/dff7b447/test/unit/org/apache/cassandra/cql3/validation/operations/SelectSingleColumnRelationTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/operations/SelectSingleColumnRelationTest.java b/test/unit/org/apache/cassandra/cql3/validation/operations/SelectSingleColumnRelationTest.java index 08bf0db..f8e5a28 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/operations/SelectSingleColumnRelationTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/operations/SelectSingleColumnRelationTest.java @@ -347,6 +347,10 @@ public class SelectSingleColumnRelationTest extends CQLTester assertInvalidMessage("IN restrictions are not supported on indexed columns", "SELECT v1 FROM %s WHERE id2 = 0 and time IN (1, 2) ALLOW FILTERING"); + + assertRows(execute("SELECT v1 FROM %s WHERE author > 'ted' AND time = 1 ALLOW FILTERING"), row("E")); + assertRows(execute("SELECT v1 FROM %s WHERE author > 'amy' AND author < 'zoe' AND time = 0 ALLOW FILTERING"), + row("A"), row("D")); } @Test @@ -605,4 +609,4 @@ public class SelectSingleColumnRelationTest extends CQLTester assertInvalidMessage("Aliases aren't allowed in the where clause ('d CONTAINS KEY 0')", "SELECT c AS d FROM %s WHERE d CONTAINS KEY 0"); assertInvalidMessage("Undefined name d in selection clause", "SELECT d FROM %s WHERE a = 0"); } -} \ No newline at end of file +}
