Repository: cassandra Updated Branches: refs/heads/cassandra-3.0 0d5984b9d -> a91219e12 refs/heads/trunk b2352189f -> bdd8e8ade
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-3.0 Commit: 1d2d0749a8b72c4c8cdd5b85b210157e8d7d6a41 Parents: 72acbcd Author: Carl Yeksigian <c...@apache.org> Authored: Tue Jun 14 08:32:26 2016 -0400 Committer: Carl Yeksigian <c...@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
