Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from
+1,176 −60
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
003933c
CASSANDRA-20667: Accord: BEGIN TRANSACTIONs IF condition logic does n…
dcapwell May 21, 2025
c0c899d
improved test
dcapwell May 21, 2025
ab4bdd5
cleanup
dcapwell May 21, 2025
809a4dc
remove todo
dcapwell May 21, 2025
364b2cb
remove dead code
dcapwell May 21, 2025
6f71245
added TimeUUIDType.instance to the allow list and added new test to d…
dcapwell May 21, 2025
b4ed52e
only multi cell lists require timestamp
dcapwell May 21, 2025
9740e1c
AccordResult swallows exceptions, so log them
dcapwell May 21, 2025
b831784
add list/map/set insert tests that show <empty> is allowed
dcapwell May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion src/java/org/apache/cassandra/cql3/conditions/ColumnCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ protected Bound(ColumnMetadata column, TableMetadata table, Operator operator, B
*/
public abstract boolean appliesTo(Row row);

public abstract boolean isNull(Row row);

public abstract BoundKind kind();

public static final ParameterisedUnversionedSerializer<Bound, TableMetadatas> serializer = new ParameterisedUnversionedSerializer<>() {
Expand Down Expand Up @@ -294,6 +296,12 @@ public boolean appliesTo(Row row)
return operator.isSatisfiedBy(column.type, rowValue(row), value);
}

@Override
public boolean isNull(Row row)
{
return column.type.isNull(rowValue(row));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNull has the following semantics

  1. for non-meangingless emptyness null and empty are both null
  2. for meangingless emptyness null and empty are both null
  3. 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

}

protected ByteBuffer rowValue(Row row)
{
// If we're asking for a given cell, and we didn't get any row from our read, it's
Expand Down Expand Up @@ -399,10 +407,21 @@ public BoundKind kind()
@Override
public boolean appliesTo(Row row)
{
ByteBuffer element = ((MultiElementType<?>) column.type).getElement(columnData(row), keyOrIndex);
ByteBuffer element = elementValue(row);
return operator.isSatisfiedBy(elementType, element, value);
}

private ByteBuffer elementValue(Row row)
{
return ((MultiElementType<?>) column.type).getElement(columnData(row), keyOrIndex);
}

@Override
public boolean isNull(Row row)
{
return column.type.isNull(elementValue(row));
}

/**
* Returns the column data for the given row.
* @param row the row
Expand Down Expand Up @@ -454,6 +473,12 @@ public boolean appliesTo(Row row)
return operator.isSatisfiedBy((MultiElementType<?>) column.type, columnData, value);
}

@Override
public boolean isNull(Row row)
{
return row == null || row.getComplexColumnData(column) == null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@Override
public boolean equals(Object o)
{
Expand Down
6 changes: 3 additions & 3 deletions src/java/org/apache/cassandra/cql3/terms/Lists.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ public Setter(ColumnMetadata column, Term t)
@Override
public boolean requiresTimestamp()
{
return true;
return column.type.isMultiCell();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

public void execute(DecoratedKey partitionKey, UpdateParameters params) throws InvalidRequestException
Expand Down Expand Up @@ -397,7 +397,7 @@ public void execute(DecoratedKey partitionKey, UpdateParameters params) throws I
@Override
public boolean requiresTimestamp()
{
return true;
return column.type.isMultiCell();
}

static void doAppend(Term.Terminal value, ColumnMetadata column, UpdateParameters params) throws InvalidRequestException
Expand Down Expand Up @@ -455,7 +455,7 @@ public Prepender(ColumnMetadata column, Term t)
@Override
public boolean requiresTimestamp()
{
return true;
return column.type.isMultiCell();
}

public void execute(DecoratedKey partitionKey, UpdateParameters params) throws InvalidRequestException
Expand Down
3 changes: 3 additions & 0 deletions src/java/org/apache/cassandra/service/accord/AccordResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.apache.cassandra.service.accord.api.PartitionKey;
import org.apache.cassandra.service.accord.txn.RetryWithNewProtocolResult;
import org.apache.cassandra.tracing.Tracing;
import org.apache.cassandra.utils.JVMStabilityInspector;
import org.apache.cassandra.utils.concurrent.AsyncFuture;

import static org.apache.cassandra.utils.Clock.Global.nanoTime;
Expand Down Expand Up @@ -142,6 +143,8 @@ else if (isTxnRequest && coordinationFailed instanceof TopologyMismatch)
}
else
{
logger.error("Unexpected exception", fail);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

JVMStabilityInspector.inspectThrowable(fail);
report = bookkeeping.newFailed(txnId, keysOrRanges);
}
report.addSuppressed(fail);
Expand Down
145 changes: 104 additions & 41 deletions src/java/org/apache/cassandra/service/accord/txn/TxnCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.Collection;
import java.util.EnumSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;
Expand All @@ -38,6 +39,7 @@
import org.apache.cassandra.db.Clustering;
import org.apache.cassandra.db.TypeSizes;
import org.apache.cassandra.db.marshal.AbstractType;
import org.apache.cassandra.db.marshal.CollectionType;
import org.apache.cassandra.db.marshal.UserType;
import org.apache.cassandra.db.partitions.FilteredPartition;
import org.apache.cassandra.db.rows.Cell;
Expand Down Expand Up @@ -289,54 +291,99 @@ public boolean applies(TxnData data)
FilteredPartition partition = reference.getPartition(data);
boolean exists = partition != null && !partition.isEmpty();

Row row = null;
if (exists)
if (reference.column() != null
&& !reference.column().isPartitionKey())
{
row = reference.getRow(partition);
exists = row != null && !row.isEmpty();
}

if (exists && reference.selectsColumn())
{
ColumnData columnData = reference.getColumnData(row);

if (columnData == null)
Row row = null;
if (exists)
{
exists = false;
row = reference.getRow(partition);
exists = row != null && !row.isEmpty();
}
else if (columnData.column().isComplex())

if (exists && reference.selectsColumn())
{
if (reference.isElementSelection() || reference.isFieldSelection())
ColumnData columnData = reference.getColumnData(row);

if (columnData == null)
{
exists = false;
}
else if (columnData.column().isComplex())
{
if (reference.isElementSelection())
{
Cell<?> cell = (Cell<?>) columnData;
exists = !cell.isTombstone();
// Collections don't support NULL but meangingless null types are supported, so byte[0] is allowed!
// This is NULL when touched, so need to still check each value
if (exists)
{
CollectionType<?> type = (CollectionType<?>) reference.column().type.unwrap();
switch (type.kind)
{
case MAP:
{
exists = !type.nameComparator().isNull(cell.path().get(0));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't thought about this in a while, but can we just reject the txn if we use a meaningless key? If we can't, then agree... map[null] on a map w/ int keys is nonsense and should evaluate to true on IS NULL

if (exists)
exists = !type.valueComparator().isNull(cell.buffer());
}
break;
case SET:
{
exists = !type.nameComparator().isNull(cell.path().get(0));
}
break;
case LIST:
{
exists = !type.valueComparator().isNull(cell.buffer());
}
break;
default:
throw new UnsupportedOperationException(type.kind.name());
}
}
}
else if (reference.isFieldSelection())
{
Cell<?> cell = (Cell<?>) columnData;
exists = exists(cell, reference.getFieldSelectionType());
}
else
{
// TODO: Is this even necessary, given the partition is already filtered?
if (!((ComplexColumnData) columnData).complexDeletion().isLive())
exists = false;
}
}
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())
Comment on lines +359 to +370
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
// This is frozen, so check if the Cell is a tombstone and that the field is present.
Cell<?> cell = (Cell<?>) columnData;
exists = !cell.isTombstone();
exists = exists(cell, reference.column().type);
if (exists)
{
ByteBuffer fieldValue = reference.getFrozenFieldValue(cell);
exists = !reference.getFieldSelectionType().isNull(fieldValue);
}
}
else
{
// TODO: Is this even necessary, given the partition is already filtered?
if (!((ComplexColumnData) columnData).complexDeletion().isLive())
exists = false;
Cell<?> cell = (Cell<?>) columnData;
exists = exists(cell, reference.column().type);
}
}
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;
ByteBuffer element = reference.getFrozenCollectionElement(cell);
exists = element != null && !cell.isTombstone();
}
else if (reference.isFieldSelection())
{
// This is frozen, so check if the Cell is a tombstone and that the field is present.
Cell<?> cell = (Cell<?>) columnData;
ByteBuffer fieldValue = reference.getFrozenFieldValue(cell);
exists = fieldValue != null && !cell.isTombstone();
}
else
{
Cell<?> cell = (Cell<?>) columnData;
exists = !cell.isTombstone();
}
}

switch (kind())
Expand All @@ -350,6 +397,11 @@ else if (reference.isFieldSelection())
}
}

private static boolean exists(Cell<?> cell, AbstractType<?> type)
{
return !cell.isTombstone() && !type.unwrap().isNull(cell.buffer());
}

private static final ConditionSerializer<Exists> serializer = new ConditionSerializer<Exists>()
{
@Override
Expand Down Expand Up @@ -442,9 +494,9 @@ public long serializedSize(ColumnConditionsAdapter condition, TableMetadatas tab

public static class Value extends TxnCondition
{
private static final Set<Kind> KINDS = ImmutableSet.of(Kind.EQUAL, Kind.NOT_EQUAL,
Kind.GREATER_THAN, Kind.GREATER_THAN_OR_EQUAL,
Kind.LESS_THAN, Kind.LESS_THAN_OR_EQUAL);
private static final EnumSet<Kind> KINDS = EnumSet.of(Kind.EQUAL, Kind.NOT_EQUAL,
Kind.GREATER_THAN, Kind.GREATER_THAN_OR_EQUAL,
Kind.LESS_THAN, Kind.LESS_THAN_OR_EQUAL);

private final TxnReference reference;
private final ByteBuffer value;
Expand All @@ -460,6 +512,11 @@ public Value(TxnReference reference, Kind kind, ByteBuffer value, ProtocolVersio
this.version = version;
}

public static EnumSet<Kind> supported()
{
return EnumSet.copyOf(KINDS);
}

@Override
public boolean equals(Object o)
{
Expand Down Expand Up @@ -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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return false;
Row row = reference.getRow(data);
if (bounds.isNull(row))
return false;
return bounds.appliesTo(row);
}

private static final ConditionSerializer<Value> serializer = new ConditionSerializer<>()
Expand Down
Loading