cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ble...@apache.org
Subject [1/2] cassandra git commit: Fix secondary index queries regression
Date Fri, 27 Jan 2017 15:27:36 GMT
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.lerer@gmail.com>
Authored: Fri Jan 27 16:06:25 2017 +0100
Committer: Benjamin Lerer <b.lerer@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()),


Mime
View raw message