This is an automated email from the ASF dual-hosted git repository.
dcapwell pushed a commit to branch cassandra-4.1
in repository https://gitbox.apache.org/repos/asf/cassandra.git
The following commit(s) were added to refs/heads/cassandra-4.1 by this push:
new d25adb498a upsert with adder support is not consistent with numbers
and strings in LWT
d25adb498a is described below
commit d25adb498abde240cc162cfe4a9630c01381c7f9
Author: David Capwell <[email protected]>
AuthorDate: Tue Sep 6 16:47:04 2022 -0700
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 +
src/java/org/apache/cassandra/cql3/Constants.java | 15 +--
.../cassandra/distributed/test/CASAddTest.java | 43 +++++++
.../cassandra/distributed/test/TestBaseImpl.java | 10 ++
.../cql3/validation/operations/UpdateTest.java | 12 ++
.../org/apache/cassandra/utils/AssertionUtils.java | 124 +++++++++++++++++++++
.../apache/cassandra/utils/AssertionUtilsTest.java | 45 ++++++++
7 files changed, 241 insertions(+), 9 deletions(-)
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<Number> type =
(NumberType<Number>) 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<Object> is(Class<?> klass)
+ {
+ String name = klass.getCanonicalName();
+ return new Condition<Object>() {
+ @Override
+ public boolean matches(Object value)
+ {
+ return value.getClass().getCanonicalName().equals(name);
+ }
+
+ @Override
+ public String toString()
+ {
+ return name;
+ }
+ };
+ }
+
+ public static <T extends Throwable> Condition<Throwable>
isThrowable(Class<T> klass)
+ {
+ // org.assertj.core.api.AbstractAssert.is has <? super ? extends
Throwable> which blocks <T>, so need to
+ // always return Throwable
+ return (Condition<Throwable>) (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<Object> isInstanceof(Class<?> klass)
+ {
+ String name = klass.getCanonicalName();
+ return new Condition<Object>() {
+ @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<Throwable> rootCause(Condition<Throwable> other)
+ {
+ return new Condition<Throwable>() {
+ @Override
+ public boolean matches(Throwable value)
+ {
+ return other.matches(Throwables.getRootCause(value));
+ }
+
+ @Override
+ public String toString()
+ {
+ return "Root cause " + other;
+ }
+ };
+ }
+
+ public static Condition<Throwable> rootCauseIs(Class<? extends Throwable>
klass)
+ {
+ return rootCause((Condition<Throwable>) (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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
