Batch with multiple conditional updates for the same partition causes AssertionError
patch by Sylvain Lebresne; reviewed by Benjamin Lerer for CASSANDRA-12867 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/78fdfe23 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/78fdfe23 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/78fdfe23 Branch: refs/heads/trunk Commit: 78fdfe2336048ba37a8b4c321ee4ab5d7bfb1357 Parents: f7aa371 Author: Sylvain Lebresne <sylv...@datastax.com> Authored: Wed Nov 2 14:47:42 2016 +0100 Committer: Sylvain Lebresne <sylv...@datastax.com> Committed: Mon Nov 7 14:09:22 2016 +0100 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../cql3/statements/CQL3CasRequest.java | 27 ++++- .../operations/InsertUpdateIfConditionTest.java | 104 +++++++++++++++++++ 3 files changed, 127 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/78fdfe23/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index f708602..1d2c8f3 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.0.10 + * Batch with multiple conditional updates for the same partition causes AssertionError (CASSANDRA-12867) * Make AbstractReplicationStrategy extendable from outside its package (CASSANDRA-12788) * Fix CommitLogTest.testDeleteIfNotDirty (CASSANDRA-12854) * Don't tell users to turn off consistent rangemovements during rebuild. (CASSANDRA-12296) http://git-wip-us.apache.org/repos/asf/cassandra/blob/78fdfe23/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java b/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java index cf4110c..d9e8796 100644 --- a/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java +++ b/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java @@ -93,12 +93,29 @@ public class CQL3CasRequest implements CASRequest { assert condition instanceof ExistCondition || condition instanceof NotExistCondition; RowCondition previous = getConditionsForRow(clustering); - if (previous != null && !(previous.getClass().equals(condition.getClass()))) + if (previous != null) { - // these should be prevented by the parser, but it doesn't hurt to check - throw (previous instanceof NotExistCondition || previous instanceof ExistCondition) - ? new InvalidRequestException("Cannot mix IF EXISTS and IF NOT EXISTS conditions for the same row") - : new InvalidRequestException("Cannot mix IF conditions and IF " + (isNotExist ? "NOT " : "") + "EXISTS for the same row"); + if (previous.getClass().equals(condition.getClass())) + { + // We can get here if a BATCH has 2 different statements on the same row with the same "exist" condition. + // For instance (assuming 'k' is the full PK): + // BEGIN BATCH + // INSERT INTO t(k, v1) VALUES (0, 'foo') IF NOT EXISTS; + // INSERT INTO t(k, v2) VALUES (0, 'bar') IF NOT EXISTS; + // APPLY BATCH; + // Of course, those can be trivially rewritten by the user as a single INSERT statement, but we still don't + // want this to be a problem (see #12867 in particular), so we simply return (the condition itself has + // already be set). + assert hasExists; // We shouldn't have a previous condition unless hasExists has been set already. + return; + } + else + { + // these should be prevented by the parser, but it doesn't hurt to check + throw (previous instanceof NotExistCondition || previous instanceof ExistCondition) + ? new InvalidRequestException("Cannot mix IF EXISTS and IF NOT EXISTS conditions for the same row") + : new InvalidRequestException("Cannot mix IF conditions and IF " + (isNotExist ? "NOT " : "") + "EXISTS for the same row"); + } } setConditionsForRow(clustering, condition); http://git-wip-us.apache.org/repos/asf/cassandra/blob/78fdfe23/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 352100e..40db977 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java @@ -1421,4 +1421,108 @@ public class InsertUpdateIfConditionTest extends CQLTester assertRows(execute("SELECT * FROM %s WHERE a = 7"), row(7, 7, null, null, 7)); } + + /** + * Test for CASSANDRA-12060, using a table without clustering. + */ + @Test + public void testMultiExistConditionOnSameRowNoClustering() throws Throwable + { + createTable("CREATE TABLE %s (k int PRIMARY KEY, v1 text, v2 text)"); + + // Multiple inserts on the same row with not exist conditions + assertRows(execute("BEGIN BATCH " + + "INSERT INTO %1$s (k, v1) values (0, 'foo') IF NOT EXISTS; " + + "INSERT INTO %1$s (k, v2) values (0, 'bar') IF NOT EXISTS; " + + "APPLY BATCH"), + row(true)); + + assertRows(execute("SELECT * FROM %s WHERE k = 0"), row(0, "foo", "bar")); + + // Same, but both insert on the same column: doing so would almost surely be a user error, but that's the + // original case reported in #12867, so being thorough. + assertRows(execute("BEGIN BATCH " + + "INSERT INTO %1$s (k, v1) values (1, 'foo') IF NOT EXISTS; " + + "INSERT INTO %1$s (k, v1) values (1, 'bar') IF NOT EXISTS; " + + "APPLY BATCH"), + row(true)); + + // As all statement gets the same timestamp, the biggest value ends up winning, so that's "foo" + assertRows(execute("SELECT * FROM %s WHERE k = 1"), row(1, "foo", null)); + + // Multiple deletes on the same row with exists conditions (note that this is somewhat non-sensical, one of the + // delete is redundant, we're just checking it doesn't break something) + assertRows(execute("BEGIN BATCH " + + "DELETE FROM %1$s WHERE k = 0 IF EXISTS; " + + "DELETE FROM %1$s WHERE k = 0 IF EXISTS; " + + "APPLY BATCH"), + row(true)); + + assertEmpty(execute("SELECT * FROM %s WHERE k = 0")); + + // Validate we can't mix different type of conditions however + assertInvalidMessage("Cannot mix IF EXISTS and IF NOT EXISTS conditions for the same row", + "BEGIN BATCH " + + "INSERT INTO %1$s (k, v1) values (1, 'foo') IF NOT EXISTS; " + + "DELETE FROM %1$s WHERE k = 1 IF EXISTS; " + + "APPLY BATCH"); + + assertInvalidMessage("Cannot mix IF conditions and IF NOT EXISTS for the same row", + "BEGIN BATCH " + + "INSERT INTO %1$s (k, v1) values (1, 'foo') IF NOT EXISTS; " + + "UPDATE %1$s SET v2 = 'bar' WHERE k = 1 IF v1 = 'foo'; " + + "APPLY BATCH"); + } + + /** + * Test for CASSANDRA-12060, using a table with clustering. + */ + @Test + public void testMultiExistConditionOnSameRowClustering() throws Throwable + { + createTable("CREATE TABLE %s (k int, t int, v1 text, v2 text, PRIMARY KEY (k, t))"); + + // Multiple inserts on the same row with not exist conditions + assertRows(execute("BEGIN BATCH " + + "INSERT INTO %1$s (k, t, v1) values (0, 0, 'foo') IF NOT EXISTS; " + + "INSERT INTO %1$s (k, t, v2) values (0, 0, 'bar') IF NOT EXISTS; " + + "APPLY BATCH"), + row(true)); + + assertRows(execute("SELECT * FROM %s WHERE k = 0"), row(0, 0, "foo", "bar")); + + // Same, but both insert on the same column: doing so would almost surely be a user error, but that's the + // original case reported in #12867, so being thorough. + assertRows(execute("BEGIN BATCH " + + "INSERT INTO %1$s (k, t, v1) values (1, 0, 'foo') IF NOT EXISTS; " + + "INSERT INTO %1$s (k, t, v1) values (1, 0, 'bar') IF NOT EXISTS; " + + "APPLY BATCH"), + row(true)); + + // As all statement gets the same timestamp, the biggest value ends up winning, so that's "foo" + assertRows(execute("SELECT * FROM %s WHERE k = 1"), row(1, 0, "foo", null)); + + // Multiple deletes on the same row with exists conditions (note that this is somewhat non-sensical, one of the + // delete is redundant, we're just checking it doesn't break something) + assertRows(execute("BEGIN BATCH " + + "DELETE FROM %1$s WHERE k = 0 AND t = 0 IF EXISTS; " + + "DELETE FROM %1$s WHERE k = 0 AND t = 0 IF EXISTS; " + + "APPLY BATCH"), + row(true)); + + assertEmpty(execute("SELECT * FROM %s WHERE k = 0")); + + // Validate we can't mix different type of conditions however + assertInvalidMessage("Cannot mix IF EXISTS and IF NOT EXISTS conditions for the same row", + "BEGIN BATCH " + + "INSERT INTO %1$s (k, t, v1) values (1, 0, 'foo') IF NOT EXISTS; " + + "DELETE FROM %1$s WHERE k = 1 AND t = 0 IF EXISTS; " + + "APPLY BATCH"); + + assertInvalidMessage("Cannot mix IF conditions and IF NOT EXISTS for the same row", + "BEGIN BATCH " + + "INSERT INTO %1$s (k, t, v1) values (1, 0, 'foo') IF NOT EXISTS; " + + "UPDATE %1$s SET v2 = 'bar' WHERE k = 1 AND t = 0 IF v1 = 'foo'; " + + "APPLY BATCH"); + } }
