cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From c...@apache.org
Subject [2/3] cassandra git commit: Allow LWT operation on static column with only partition keys
Date Tue, 14 Jun 2016 14:26:38 GMT
Allow LWT operation on static column with only partition keys

Patch by Carl Yeksigian, reviewed by Benjamin Lerer for CASSANDRA-10532


Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/1d2d0749
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/1d2d0749
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/1d2d0749

Branch: refs/heads/cassandra-2.2
Commit: 1d2d0749a8b72c4c8cdd5b85b210157e8d7d6a41
Parents: 72acbcd
Author: Carl Yeksigian <carl@apache.org>
Authored: Tue Jun 14 08:32:26 2016 -0400
Committer: Carl Yeksigian <carl@apache.org>
Committed: Tue Jun 14 08:32:26 2016 -0400

----------------------------------------------------------------------
 CHANGES.txt                                     |   1 +
 .../cql3/statements/DeleteStatement.java        |  37 +++---
 .../cql3/statements/ModificationStatement.java  |  14 ++-
 .../operations/InsertUpdateIfConditionTest.java | 113 +++++++++++++++++++
 4 files changed, 150 insertions(+), 15 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/1d2d0749/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index ebcc90c..7d70902 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,4 +1,5 @@
 2.1.15
+ * Allow LWT operation on static column with only partition keys (CASSANDRA-10532)
  * Create interval tree over canonical sstables to avoid missing sstables during streaming
(CASSANDRA-11886)
  * cqlsh COPY FROM: shutdown parent cluster after forking, to avoid corrupting SSL connections
(CASSANDRA-11749)
  * Updated cqlsh Python driver to fix DESCRIBE problem for legacy tables (CASSANDRA-11055)

http://git-wip-us.apache.org/repos/asf/cassandra/blob/1d2d0749/src/java/org/apache/cassandra/cql3/statements/DeleteStatement.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql3/statements/DeleteStatement.java b/src/java/org/apache/cassandra/cql3/statements/DeleteStatement.java
index 33c61e7..d8fa467 100644
--- a/src/java/org/apache/cassandra/cql3/statements/DeleteStatement.java
+++ b/src/java/org/apache/cassandra/cql3/statements/DeleteStatement.java
@@ -50,15 +50,6 @@ public class DeleteStatement extends ModificationStatement
     {
         List<Operation> deletions = getOperations();
 
-        if (prefix.size() < cfm.clusteringColumns().size() && !deletions.isEmpty())
-        {
-            // In general, we can't delete specific columns if not all clustering columns
have been specified.
-            // However, if we delete only static colums, it's fine since we won't really
use the prefix anyway.
-            for (Operation deletion : deletions)
-                if (!deletion.column.isStatic())
-                    throw new InvalidRequestException(String.format("Primary key column '%s'
must be specified in order to delete column '%s'", getFirstEmptyKey().name, deletion.column.name));
-        }
-
         if (deletions.isEmpty())
         {
             // We delete the slice selected by the prefix.
@@ -88,19 +79,39 @@ public class DeleteStatement extends ModificationStatement
 
     protected void validateWhereClauseForConditions() throws InvalidRequestException
     {
-        Iterator<ColumnDefinition> iterator = Iterators.concat(cfm.partitionKeyColumns().iterator(),
cfm.clusteringColumns().iterator());
+        boolean onlyHasConditionsOnStaticColumns = hasStaticConditions() && !hasRegularConditions();
+
+        // In general, we can't delete specific columns if not all clustering columns have
been specified.
+        // However, if we delete only static colums, it's fine since we won't really use
the prefix anyway.
+        Iterator<ColumnDefinition> iterator = appliesOnlyToStaticColumns()
+                                              ? cfm.partitionKeyColumns().iterator()
+                                              : Iterators.concat(cfm.partitionKeyColumns().iterator(),
cfm.clusteringColumns().iterator());
         while (iterator.hasNext())
         {
             ColumnDefinition def = iterator.next();
             Restriction restriction = processedKeys.get(def.name);
             if (restriction == null || !(restriction.isEQ() || restriction.isIN()))
             {
+                if (onlyHasConditionsOnStaticColumns)
+                {
+                    for (Operation oper : getOperations())
+                    {
+                        if (!oper.column.isStatic())
+                        {
+                            throw new InvalidRequestException(String.format("Primary key
column '%s' must be specified in order to delete column '%s'",
+                                                                            def.name,
+                                                                            oper.column.name));
+                        }
+                    }
+                }
+
                 throw new InvalidRequestException(
-                        String.format("DELETE statements must restrict all PRIMARY KEY columns
with equality relations in order " +
-                                      "to use IF conditions, but column '%s' is not restricted",
def.name));
+                        String.format("DELETE statements must restrict all %s KEY columns
with equality relations in order " +
+                                      "to use IF conditions%s, but column '%s' is not restricted",
+                                      onlyHasConditionsOnStaticColumns ? "PARTITION" : "PRIMARY",
+                                      onlyHasConditionsOnStaticColumns ? " on static columns"
: "", def.name));
             }
         }
-
     }
 
     public static class Parsed extends ModificationStatement.Parsed

http://git-wip-us.apache.org/repos/asf/cassandra/blob/1d2d0749/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 75a3b40..a9f65e1 100644
--- a/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java
+++ b/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java
@@ -211,6 +211,16 @@ public abstract class ModificationStatement implements CQLStatement
         return ifExists;
     }
 
+    public boolean hasStaticConditions()
+    {
+        return staticConditions != null && !staticConditions.isEmpty();
+    }
+
+    public boolean hasRegularConditions()
+    {
+        return columnConditions != null && !columnConditions.isEmpty();
+    }
+
     private void addKeyValues(ColumnDefinition def, Restriction values) throws InvalidRequestException
     {
         if (def.kind == ColumnDefinition.Kind.CLUSTERING_COLUMN)
@@ -364,7 +374,7 @@ public abstract class ModificationStatement implements CQLStatement
      * Checks that the modification only apply to static columns.
      * @return <code>true</code> if the modification only apply to static columns,
<code>false</code> otherwise.
      */
-    private boolean appliesOnlyToStaticColumns()
+    protected boolean appliesOnlyToStaticColumns()
     {
         return setsStaticColumns && !appliesToRegularColumns();
     }
@@ -373,7 +383,7 @@ public abstract class ModificationStatement implements CQLStatement
      * Checks that the modification apply to regular columns.
      * @return <code>true</code> if the modification apply to regular columns,
<code>false</code> otherwise.
      */
-    private boolean appliesToRegularColumns()
+    protected boolean appliesToRegularColumns()
     {
         // If we have regular operations, this applies to regular columns.
         // Otherwise, if the statement is a DELETE and columnOperations is empty, this means
we have no operations,

http://git-wip-us.apache.org/repos/asf/cassandra/blob/1d2d0749/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java
b/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java
index e94011b..05ba09d 100644
--- a/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java
+++ b/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java
@@ -311,6 +311,119 @@ public class InsertUpdateIfConditionTest extends CQLTester
     }
 
     /**
+     * Test CASSANDRA-10532
+     */
+    @Test
+    public void testStaticColumnsCasDelete() throws Throwable
+    {
+        createTable("CREATE TABLE %s (pk int, ck int, static_col int static, value int, PRIMARY
KEY (pk, ck))");
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 1, 2);
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 3, 4);
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 5, 6);
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 7, 8);
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 2, 1, 2);
+        execute("INSERT INTO %s (pk, static_col) VALUES (?, ?)", 1, 1);
+
+        assertRows(execute("DELETE static_col FROM %s WHERE pk = ? IF static_col = ?", 1,
2), row(false, 1));
+        assertRows(execute("DELETE static_col FROM %s WHERE pk = ? IF static_col = ?", 1,
1), row(true));
+
+        assertRows(execute("SELECT pk, ck, static_col, value FROM %s WHERE pk = 1"),
+                   row(1, 1, null, 2),
+                   row(1, 3, null, 4),
+                   row(1, 5, null, 6),
+                   row(1, 7, null, 8));
+        execute("INSERT INTO %s (pk, static_col) VALUES (?, ?)", 1, 1);
+
+        assertInvalidMessage("DELETE statements must restrict all PARTITION KEY columns with
equality relations in order " +
+                             "to use IF conditions on static columns, but column 'pk' is
not restricted",
+                             "DELETE static_col FROM %s WHERE ck = ? IF static_col = ?",
1, 1);
+
+        assertInvalidMessage("Invalid restriction on clustering column ck since the DELETE
statement modifies only static columns",
+                             "DELETE static_col FROM %s WHERE pk = ? AND ck = ? IF static_col
= ?", 1, 1, 1);
+
+        assertInvalidMessage("Primary key column 'ck' must be specified in order to delete
column 'value'",
+                             "DELETE static_col, value FROM %s WHERE pk = ? IF static_col
= ?", 1, 1);
+
+        // Same query but with an invalid condition
+        assertInvalidMessage("Primary key column 'ck' must be specified in order to delete
column 'value'",
+                             "DELETE static_col, value FROM %s WHERE pk = ? IF static_col
= ?", 1, 2);
+
+        // DELETE of an underspecified PRIMARY KEY should not succeed if static is not only
restriction
+        assertInvalidMessage("DELETE statements must restrict all PRIMARY KEY columns with
equality relations in order " +
+                             "to use IF conditions, but column 'ck' is not restricted",
+                             "DELETE static_col FROM %s WHERE pk = ? IF value = ? AND static_col
= ?", 1, 2, 1);
+
+        assertRows(execute("DELETE value FROM %s WHERE pk = ? AND ck = ? IF value = ? AND
static_col = ?", 1, 1, 2, 2), row(false, 2, 1));
+        assertRows(execute("DELETE value FROM %s WHERE pk = ? AND ck = ? IF value = ? AND
static_col = ?", 1, 1, 2, 1), row(true));
+        assertRows(execute("SELECT pk, ck, static_col, value FROM %s WHERE pk = 1"),
+                   row(1, 1, 1, null),
+                   row(1, 3, 1, 4),
+                   row(1, 5, 1, 6),
+                   row(1, 7, 1, 8));
+
+        assertRows(execute("DELETE static_col FROM %s WHERE pk = ? AND ck = ? IF value =
?", 1, 5, 10), row(false, 6));
+        assertRows(execute("DELETE static_col FROM %s WHERE pk = ? AND ck = ? IF value =
?", 1, 5, 6), row(true));
+        assertRows(execute("SELECT pk, ck, static_col, value FROM %s WHERE pk = 1"),
+                   row(1, 1, null, null),
+                   row(1, 3, null, 4),
+                   row(1, 5, null, 6),
+                   row(1, 7, null, 8));
+    }
+
+    @Test
+    public void testStaticColumnsCasUpdate() throws Throwable
+    {
+        createTable("CREATE TABLE %s (pk int, ck int, static_col int static, value int, PRIMARY
KEY (pk, ck))");
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 1, 2);
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 3, 4);
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 5, 6);
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 7, 8);
+        execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 2, 1, 2);
+        execute("INSERT INTO %s (pk, static_col) VALUES (?, ?)", 1, 1);
+
+        assertRows(execute("UPDATE %s SET static_col = ? WHERE pk = ? IF static_col = ?",
3, 1, 2), row(false, 1));
+        assertRows(execute("UPDATE %s SET static_col = ? WHERE pk = ? IF static_col = ?",
2, 1, 1), row(true));
+
+        assertRows(execute("SELECT pk, ck, static_col, value FROM %s WHERE pk = 1"),
+                   row(1, 1, 2, 2),
+                   row(1, 3, 2, 4),
+                   row(1, 5, 2, 6),
+                   row(1, 7, 2, 8));
+
+        assertInvalidMessage("Missing mandatory PRIMARY KEY part pk",
+                             "UPDATE %s SET static_col = ? WHERE ck = ? IF static_col = ?",
3, 1, 1);
+
+        assertInvalidMessage("Invalid restriction on clustering column ck since the UPDATE
statement modifies only static columns",
+                             "UPDATE %s SET static_col = ? WHERE pk = ? AND ck = ? IF static_col
= ?", 3, 1, 1, 1);
+
+        assertInvalidMessage("Missing mandatory PRIMARY KEY part ck",
+                             "UPDATE %s SET static_col = ?, value = ? WHERE pk = ? IF static_col
= ?", 3, 1, 1, 2);
+
+        // Same query but with an invalid condition
+        assertInvalidMessage("Missing mandatory PRIMARY KEY part ck",
+                             "UPDATE %s SET static_col = ?, value = ? WHERE pk = ? IF static_col
= ?", 3, 1, 1, 1);
+
+        assertInvalidMessage("Missing mandatory PRIMARY KEY part ck",
+                             "UPDATE %s SET static_col = ? WHERE pk = ? IF value = ? AND
static_col = ?", 3, 1, 4, 2);
+
+        assertRows(execute("UPDATE %s SET value = ? WHERE pk = ? AND ck = ? IF value = ?
AND static_col = ?", 3, 1, 1, 3, 2), row(false, 2, 2));
+        assertRows(execute("UPDATE %s SET value = ? WHERE pk = ? AND ck = ? IF value = ?
AND static_col = ?", 1, 1, 1, 2, 2), row(true));
+        assertRows(execute("SELECT pk, ck, static_col, value FROM %s WHERE pk = 1"),
+                   row(1, 1, 2, 1),
+                   row(1, 3, 2, 4),
+                   row(1, 5, 2, 6),
+                   row(1, 7, 2, 8));
+
+        assertRows(execute("UPDATE %s SET static_col = ? WHERE pk = ? AND ck = ? IF value
= ?", 3, 1, 1, 2), row(false, 1));
+        assertRows(execute("UPDATE %s SET static_col = ? WHERE pk = ? AND ck = ? IF value
= ?", 1, 1, 1, 1), row(true));
+        assertRows(execute("SELECT pk, ck, static_col, value FROM %s WHERE pk = 1"),
+                   row(1, 1, 1, 1),
+                   row(1, 3, 1, 4),
+                   row(1, 5, 1, 6),
+                   row(1, 7, 1, 8));
+    }
+
+    /**
      * Migrated from cql_tests.py:TestCQL.bug_6069_test()
      */
     @Test


Mime
View raw message