CASSANDRA-20667: Accord: BEGIN TRANSACTIONs IF condition logic does not properly support meaningless emptiness and null values #4175

dcapwell · 2025-05-21T15:44:07Z

No description provided.

…ot properly support meaningless emptiness and null values

dcapwell · 2025-05-21T15:46:17Z

src/java/org/apache/cassandra/cql3/conditions/ColumnCondition.java

+        @Override
+        public boolean isNull(Row row)
+        {
+            return column.type.isNull(rowValue(row));


isNull has the following semantics

for non-meangingless emptyness null and empty are both null

for meangingless emptyness null and empty are both null

for types that support empty, only null is null

The main difference between 1/2 is that in 1 empty is rejected during validation, so normally means you passed a tombstone here by mistake (historic bug on the read path in RowFilter). 2 allows empty during validation

dcapwell · 2025-05-21T15:50:45Z

src/java/org/apache/cassandra/service/accord/txn/TxnCondition.java

+                                {
+                                    case MAP:
+                                    {
+                                        exists = !type.nameComparator().isNull(cell.path().get(0));


im still not 100% sold on this, here is the test that hits this

map<int, int> IF row.map[<empty>] IS NULL -- returns true as the key is "null". The data has map[<empty>] -> 42 IF row.map[42] IS NULL -- returns true if the value is <empty>

I don't think we have a particularly good principled behaviour we can implement here, but I think treating empty as NULL wherever it is safe to do so, and unsafe to treat it as a raw value, is perhaps as good as we can get.

dcapwell · 2025-05-21T15:53:21Z

src/java/org/apache/cassandra/service/accord/txn/TxnCondition.java

@@ -532,7 +589,13 @@ else if (type.isUDT())
        @Override
        public boolean applies(TxnData data)
        {
-            return getBounds(data).appliesTo(reference.getRow(data));
+            Bound bounds = getBounds(data);
+            if (reference.column().type.unwrap().isNull(bounds.value))


semantic difference with CAS IF. if input or value are null, applies is

reference.column().type.unwrap().isNull(bounds.value) == bounds.isNull(row)

I think this is OK, because bound cannot be an IS NULL or IS NOT NULL check.

dcapwell · 2025-05-21T15:55:08Z

src/java/org/apache/cassandra/service/accord/txn/TxnReference.java

@@ -351,4 +407,100 @@ public long serializedSize(TxnReference reference, TableMetadatas tables, Versio
            return size;
        }
    };
+
+    private static class ClusteringColumnData extends AbstractCell<ByteBuffer>


this is kinda a hack... it was this or refactor the code more. I could also special case clustering, but felt that this was best as it kept the logic simpler

dcapwell · 2025-05-21T15:56:15Z

src/java/org/apache/cassandra/service/accord/txn/TxnReferenceValue.java

        private final ByteBuffer value;

-        public Constant(ByteBuffer value)
+        public Constant(@Nullable ByteBuffer value)


should add a test to show this, but the value can be null which leads to NPE later on, so marked this nullable and made it handled

dcapwell · 2025-05-21T15:57:02Z

src/java/org/apache/cassandra/service/accord/txn/TxnReferenceValue.java

@@ -115,19 +118,24 @@ void collect(TableMetadatas.Collector collector)
            @Override
            public void serialize(Constant constant, TableMetadatas tables, DataOutputPlus out, Version version) throws IOException
            {
-                ByteBufferUtil.writeWithVIntLength(constant.value, out);
+                out.writeBoolean(constant.value != null);


not backwards or mixed compatible

dcapwell · 2025-05-21T15:59:25Z

test/unit/org/apache/cassandra/db/partitions/SimplePartition.java

+import org.apache.cassandra.utils.memory.Cloner;
+import org.apache.cassandra.utils.memory.HeapCloner;
+
+public class SimplePartition extends AbstractBTreePartition


creating the RowIterator i need for the test was a lot of boiler plate, so figured could make this class to make writing tests less verbose

dcapwell · 2025-05-21T16:00:33Z

test/unit/org/apache/cassandra/db/partitions/SimplePartition.java

+        }
+    }
+
+    private class Updater implements UpdateFunction<Row, Row>, ColumnData.PostReconciliationFunction


honestly copied a ton from atomic btree partition, the issue i faced with reusing is having an allocator... I could try to make that logic reusable but didn't want to touch a critical section of code so I forked

…etect anything else missing

dcapwell · 2025-05-21T16:17:33Z

src/java/org/apache/cassandra/cql3/conditions/ColumnCondition.java

+        @Override
+        public boolean isNull(Row row)
+        {
+            return row == null || row.getComplexColumnData(column) == null;


this is all filtered data, so can this data be non-live?

dcapwell · 2025-05-21T16:19:28Z

src/java/org/apache/cassandra/cql3/terms/Lists.java

@@ -300,7 +300,7 @@ public Setter(ColumnMetadata column, Term t)
        @Override
        public boolean requiresTimestamp()
        {
-            return true;
+            return column.type.isMultiCell();


bug found in fuzz testing, frozen lists were requiring timestamp which made it hit this block

public void add(Operation operation, TableMetadata tableMetadata) { if (isForTxn && (operation.requiresRead() || operation.requiresTimestamp())) { add(operation.column, ReferenceOperation.create(operation, tableMetadata)); return; }

this is a immutable bb so can avoid converting to a reference (i think it fails as a reference but forgot)

dcapwell · 2025-05-21T16:21:14Z

src/java/org/apache/cassandra/service/accord/AccordResult.java

@@ -142,6 +143,8 @@ else if (isTxnRequest && coordinationFailed instanceof TopologyMismatch)
            }
            else
            {
+                logger.error("Unexpected exception", fail);


I was seeing that logs were clean but exceptions were happening, and found that this code block was hiding all these exceptions. The error isn't a CoordinationFailed so its not expected, so we should be loud! I found several unexpected exceptions by adding this in

dcapwell · 2025-05-21T16:21:56Z

src/java/org/apache/cassandra/service/accord/txn/TxnCondition.java

+                    else if (reference.isElementSelection())
+                    {
+                        // This is frozen, so check if the Cell is a tombstone and that the element is present.
+                        Cell<?> cell = (Cell<?>) columnData;
+                        exists = exists(cell, reference.column().type);
+                        if (exists)
+                        {
+                            ByteBuffer element = reference.getFrozenCollectionElement(cell);
+                            exists = !reference.getFrozenCollectionElementType().isNull(element);
+                        }
+                    }
+                    else if (reference.isFieldSelection())


im not sure if this is allowed in CQL, but it was already here in the code so i made sure it worked

CASSANDRA-20667: Accord: BEGIN TRANSACTIONs IF condition logic does n…

003933c

…ot properly support meaningless emptiness and null values

dcapwell commented May 21, 2025

View reviewed changes

dcapwell added 2 commits May 21, 2025 08:58

improved test

c0c899d

cleanup

ab4bdd5

dcapwell commented May 21, 2025

View reviewed changes

dcapwell added 6 commits May 21, 2025 09:01

remove todo

809a4dc

remove dead code

364b2cb

added TimeUUIDType.instance to the allow list and added new test to d…

6f71245

…etect anything else missing

only multi cell lists require timestamp

b4ed52e

AccordResult swallows exceptions, so log them

9740e1c

add list/map/set insert tests that show <empty> is allowed

b831784

dcapwell commented May 21, 2025

View reviewed changes

dcapwell requested a review from belliottsmith May 21, 2025 17:28

maedhroz self-requested a review June 6, 2025 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CASSANDRA-20667: Accord: BEGIN TRANSACTIONs IF condition logic does not properly support meaningless emptiness and null values #4175

CASSANDRA-20667: Accord: BEGIN TRANSACTIONs IF condition logic does not properly support meaningless emptiness and null values #4175

Uh oh!

dcapwell commented May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

belliottsmith May 30, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

belliottsmith May 30, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

dcapwell May 21, 2025

Uh oh!

Uh oh!

CASSANDRA-20667: Accord: BEGIN TRANSACTIONs IF condition logic does not properly support meaningless emptiness and null values #4175

Are you sure you want to change the base?

CASSANDRA-20667: Accord: BEGIN TRANSACTIONs IF condition logic does not properly support meaningless emptiness and null values #4175

Uh oh!

Conversation

dcapwell commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!