From d25adb498abde240cc162cfe4a9630c01381c7f9 Mon Sep 17 00:00:00 2001 From: David Capwell Date: Tue, 6 Sep 2022 16:47:04 -0700 Subject: [PATCH] upsert with adder support is not consistent with numbers and strings in LWT patch by David Capwell; reviewed by Benedict Elliott Smith, Caleb Rackliffe for CASSANDRA-17857 --- CHANGES.txt | 1 + .../org/apache/cassandra/cql3/Constants.java | 15 +-- .../distributed/test/CASAddTest.java | 43 ++++++ .../distributed/test/TestBaseImpl.java | 10 ++ .../validation/operations/UpdateTest.java | 12 ++ .../cassandra/utils/AssertionUtils.java | 124 ++++++++++++++++++ .../cassandra/utils/AssertionUtilsTest.java | 45 +++++++ 7 files changed, 241 insertions(+), 9 deletions(-) create mode 100644 test/unit/org/apache/cassandra/utils/AssertionUtils.java create mode 100644 test/unit/org/apache/cassandra/utils/AssertionUtilsTest.java diff --git a/CHANGES.txt b/CHANGES.txt index 900df894c0..1bf090a50f 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.1-alpha2 + * upsert with adder support is not consistent with numbers and strings in LWT (CASSANDRA-17857) * Fix race and return after failing connections (CASSANDRA-17618) * Speculative execution threshold unit mismatch (CASSANDRA-17877) * Fix BulkLoader to load entireSSTableThrottle and entireSSTableInterDcThrottle (CASSANDRA-17677) diff --git a/src/java/org/apache/cassandra/cql3/Constants.java b/src/java/org/apache/cassandra/cql3/Constants.java index e8989ad24e..64d9d6917e 100644 --- a/src/java/org/apache/cassandra/cql3/Constants.java +++ b/src/java/org/apache/cassandra/cql3/Constants.java @@ -485,6 +485,8 @@ public abstract class Constants @SuppressWarnings("unchecked") NumberType type = (NumberType) column.type; ByteBuffer increment = t.bindAndGet(params.options); ByteBuffer current = getCurrentCellBuffer(partitionKey, params); + if (current == null) + return; ByteBuffer newValue = type.add(type, current, type, increment); params.addCell(column, newValue); } @@ -494,15 +496,10 @@ public abstract class Constants ByteBuffer current = getCurrentCellBuffer(partitionKey, params); ByteBuffer newValue; if (current == null) - { - newValue = append; - } - else - { - newValue = ByteBuffer.allocate(current.remaining() + append.remaining()); - FastByteOperations.copy(current, current.position(), newValue, newValue.position(), current.remaining()); - FastByteOperations.copy(append, append.position(), newValue, newValue.position() + current.remaining(), append.remaining()); - } + return; + newValue = ByteBuffer.allocate(current.remaining() + append.remaining()); + FastByteOperations.copy(current, current.position(), newValue, newValue.position(), current.remaining()); + FastByteOperations.copy(append, append.position(), newValue, newValue.position() + current.remaining(), append.remaining()); params.addCell(column, newValue); } } diff --git a/test/distributed/org/apache/cassandra/distributed/test/CASAddTest.java b/test/distributed/org/apache/cassandra/distributed/test/CASAddTest.java index 59220cc24d..c50b9b526e 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/CASAddTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/CASAddTest.java @@ -24,6 +24,9 @@ import org.slf4j.LoggerFactory; import org.apache.cassandra.distributed.Cluster; import org.apache.cassandra.distributed.api.ConsistencyLevel; +import org.apache.cassandra.exceptions.InvalidRequestException; +import org.apache.cassandra.utils.AssertionUtils; +import org.assertj.core.api.Assertions; import static org.apache.cassandra.distributed.shared.AssertUtils.assertRows; import static org.apache.cassandra.distributed.shared.AssertUtils.row; @@ -62,6 +65,46 @@ public class CASAddTest extends TestBaseImpl } } + @Test + public void testAdditionNotExists() throws Throwable + { + try (Cluster cluster = init(Cluster.create(3))) + { + cluster.schemaChange("CREATE TABLE " + KEYSPACE + ".tbl (pk int PRIMARY KEY, a int, b text)"); + + // in this context partition/row not existing looks like column not existing, so to simplify the LWT required + // condition, add a row with null columns so can rely on IF EXISTS + cluster.coordinator(1).execute("INSERT INTO " + KEYSPACE + ".tbl (pk) VALUES (1)", ConsistencyLevel.QUORUM); + + // n = n + value where n = null + cluster.coordinator(1).execute("UPDATE " + KEYSPACE + ".tbl SET a = a + 1, b = b + 'fail' WHERE pk = 1 IF EXISTS", ConsistencyLevel.QUORUM); + // the SET should all no-op due to null... so should no-op + assertRows(cluster.coordinator(1).execute("SELECT * FROM " + KEYSPACE + ".tbl WHERE pk = 1", ConsistencyLevel.SERIAL), row(1, null, null)); + + // this section is testing current limitations... if they start to fail due to the limitations going away... update this test to include those cases + Assertions.assertThatThrownBy(() -> cluster.coordinator(1).execute(batch( + "INSERT INTO " + KEYSPACE + ".tbl (pk, a, b) VALUES (1, 0, '') IF NOT EXISTS", + "UPDATE " + KEYSPACE + ".tbl SET a = a + 1, b = b + 'success' WHERE pk = 1 IF EXISTS" + ), ConsistencyLevel.QUORUM)) + .is(AssertionUtils.is(InvalidRequestException.class)) + .hasMessage("Cannot mix IF EXISTS and IF NOT EXISTS conditions for the same row"); + Assertions.assertThatThrownBy(() -> cluster.coordinator(1).execute(batch( + "INSERT INTO " + KEYSPACE + ".tbl (pk, a, b) VALUES (1, 0, '') IF NOT EXISTS", + + "UPDATE " + KEYSPACE + ".tbl SET a = a + 1, b = b + 'success' WHERE pk = 1" + ), ConsistencyLevel.QUORUM)) + .is(AssertionUtils.is(InvalidRequestException.class)) + .hasMessage("Invalid operation (a = a + 1) for non counter column a"); + + // since CAS doesn't allow the above cases, manually add the data to unblock... + cluster.coordinator(1).execute("INSERT INTO " + KEYSPACE + ".tbl (pk, a, b) VALUES (1, 0, '')", ConsistencyLevel.QUORUM); + + // have cas add defaults when missing + cluster.coordinator(1).execute("UPDATE " + KEYSPACE + ".tbl SET a = a + 1, b = b + 'success' WHERE pk = 1 IF EXISTS", ConsistencyLevel.QUORUM); + assertRows(cluster.coordinator(1).execute("SELECT * FROM " + KEYSPACE + ".tbl WHERE pk = 1", ConsistencyLevel.SERIAL), row(1, 1, "success")); + } + } + @Test public void testConcat() throws Throwable { diff --git a/test/distributed/org/apache/cassandra/distributed/test/TestBaseImpl.java b/test/distributed/org/apache/cassandra/distributed/test/TestBaseImpl.java index 4755d70f8c..e97a08105f 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/TestBaseImpl.java +++ b/test/distributed/org/apache/cassandra/distributed/test/TestBaseImpl.java @@ -104,6 +104,16 @@ public class TestBaseImpl extends DistributedTestBase return TupleType.buildValue(bbs); } + public static String batch(String... queries) + { + StringBuilder sb = new StringBuilder(); + sb.append("BEGIN UNLOGGED BATCH\n"); + for (String q : queries) + sb.append(q).append(";\n"); + sb.append("APPLY BATCH;"); + return sb.toString(); + } + protected void bootstrapAndJoinNode(Cluster cluster) { IInstanceConfig config = cluster.newInstanceConfig(); diff --git a/test/unit/org/apache/cassandra/cql3/validation/operations/UpdateTest.java b/test/unit/org/apache/cassandra/cql3/validation/operations/UpdateTest.java index 027955780b..59a0616106 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/operations/UpdateTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/operations/UpdateTest.java @@ -28,6 +28,8 @@ import org.apache.cassandra.cql3.UntypedResultSet; import org.apache.cassandra.cql3.UntypedResultSet.Row; import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.db.Keyspace; +import org.apache.cassandra.exceptions.InvalidRequestException; +import org.assertj.core.api.Assertions; import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertEquals; @@ -657,4 +659,14 @@ public class UpdateTest extends CQLTester ColumnFamilyStore cfs = keyspace.getColumnFamilyStore(currentTable()); return cfs.metric.allMemtablesLiveDataSize.getValue() == 0; } + + @Test + public void testAdderNonCounter() + { + createTable("CREATE TABLE %s (pk int PRIMARY KEY, a int, b text)"); + Assertions.assertThatThrownBy(() -> execute("UPDATE %s SET a = a + 1, b = b + 'fail' WHERE pk = 1")) + .isInstanceOf(InvalidRequestException.class) + // if error ever includes "b" its safe to update this test + .hasMessage("Invalid operation (a = a + 1) for non counter column a"); + } } diff --git a/test/unit/org/apache/cassandra/utils/AssertionUtils.java b/test/unit/org/apache/cassandra/utils/AssertionUtils.java new file mode 100644 index 0000000000..d5b1981fc1 --- /dev/null +++ b/test/unit/org/apache/cassandra/utils/AssertionUtils.java @@ -0,0 +1,124 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.utils; + +import com.google.common.base.Throwables; + +import org.assertj.core.api.Condition; + +public class AssertionUtils +{ + private AssertionUtils() + { + } + + /** + * When working with jvm-dtest the thrown error is in a different {@link ClassLoader} causing type checks + * to fail; this method relies on naming instead. + */ + public static Condition is(Class klass) + { + String name = klass.getCanonicalName(); + return new Condition() { + @Override + public boolean matches(Object value) + { + return value.getClass().getCanonicalName().equals(name); + } + + @Override + public String toString() + { + return name; + } + }; + } + + public static Condition isThrowable(Class klass) + { + // org.assertj.core.api.AbstractAssert.is has which blocks , so need to + // always return Throwable + return (Condition) (Condition) is(klass); + } + + /** + * When working with jvm-dtest the thrown error is in a different {@link ClassLoader} causing type checks + * to fail; this method relies on naming instead. + * + * This method is different than {@link #is(Class)} as it tries to mimic instanceOf rather than equality. + */ + public static Condition isInstanceof(Class klass) + { + String name = klass.getCanonicalName(); + return new Condition() { + @Override + public boolean matches(Object value) + { + if (value == null) + return false; + return matches(value.getClass()); + } + + private boolean matches(Class input) + { + for (Class klass = input; klass != null; klass = klass.getSuperclass()) + { + // extends + if (klass.getCanonicalName().equals(name)) + return true; + // implements + for (Class i : klass.getInterfaces()) + { + if (matches(i)) + return true; + } + } + return false; + } + + @Override + public String toString() + { + return name; + } + }; + } + + public static Condition rootCause(Condition other) + { + return new Condition() { + @Override + public boolean matches(Throwable value) + { + return other.matches(Throwables.getRootCause(value)); + } + + @Override + public String toString() + { + return "Root cause " + other; + } + }; + } + + public static Condition rootCauseIs(Class klass) + { + return rootCause((Condition) (Condition) is(klass)); + } +} diff --git a/test/unit/org/apache/cassandra/utils/AssertionUtilsTest.java b/test/unit/org/apache/cassandra/utils/AssertionUtilsTest.java new file mode 100644 index 0000000000..e3ec93ab48 --- /dev/null +++ b/test/unit/org/apache/cassandra/utils/AssertionUtilsTest.java @@ -0,0 +1,45 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.utils; + +import org.junit.Test; + +import org.assertj.core.api.Assertions; + +public class AssertionUtilsTest +{ + @Test + public void isInstanceof() + { + Assertions.assertThat(new C()) + .is(AssertionUtils.isInstanceof(A.class)); + + Assertions.assertThat(new D()) + .is(AssertionUtils.isInstanceof(A.class)) + .is(AssertionUtils.isInstanceof(B.class)); + + Assertions.assertThat(null instanceof A) + .isEqualTo(AssertionUtils.isInstanceof(A.class).matches(null)); + } + + interface A {} + interface B extends A {} + static class C implements A {} + static class D implements B {} +} \ No newline at end of file