dcapwell commented on PR #3731:
URL: https://github.com/apache/cassandra/pull/3731#issuecomment-2560427817
I applied the patch to the branch that found the problem, and applied the
feedback I gave and the test that found the issue is passing now!
Here is the modified version of this PR with my feedback applied
```
diff --git a/src/java/org/apache/cassandra/db/filter/RowFilter.java
b/src/java/org/apache/cassandra/db/filter/RowFilter.java
index 9ea44ead96..ce96e8daa0 100644
--- a/src/java/org/apache/cassandra/db/filter/RowFilter.java
+++ b/src/java/org/apache/cassandra/db/filter/RowFilter.java
@@ -224,7 +224,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
// Short-circuit all partitions that won't match based on
static and partition keys
for (Expression e : partitionLevelExpressions)
- if (!e.isSatisfiedBy(metadata,
partition.partitionKey(), partition.staticRow()))
+ if (!e.isSatisfiedBy(metadata,
partition.partitionKey(), partition.staticRow(), nowInSec))
{
partition.close();
return null;
@@ -251,7 +251,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
return null;
for (Expression e : rowLevelExpressions)
- if (!e.isSatisfiedBy(metadata, pk, purged))
+ if (!e.isSatisfiedBy(metadata, pk, purged, nowInSec))
return null;
return row;
@@ -303,7 +303,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
for (Expression e : expressions)
{
- if (!e.isSatisfiedBy(metadata, partitionKey, purged))
+ if (!e.isSatisfiedBy(metadata, partitionKey, purged, nowInSec))
return false;
}
return true;
@@ -522,9 +522,9 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
* (i.e. it should come from a RowIterator).
* @return whether the row is satisfied by this expression.
*/
- public abstract boolean isSatisfiedBy(TableMetadata metadata,
DecoratedKey partitionKey, Row row);
+ public abstract boolean isSatisfiedBy(TableMetadata metadata,
DecoratedKey partitionKey, Row row, long nowInSec);
- protected ByteBuffer getValue(TableMetadata metadata, DecoratedKey
partitionKey, Row row)
+ protected ByteBuffer getValue(TableMetadata metadata, DecoratedKey
partitionKey, Row row, long nowInSec)
{
switch (column.kind)
{
@@ -536,7 +536,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
return row.clustering().bufferAt(column.position());
default:
Cell<?> cell = row.getCell(column);
- return cell == null ? null : cell.buffer();
+ return Cell.getValidCellBuffer(cell, nowInSec);
}
}
@@ -697,7 +697,8 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
super(column, operator, value);
}
- public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row)
+ @Override
+ public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row, long nowInSec)
{
// We support null conditions for LWT (in ColumnCondition) but
not for RowFilter.
// TODO: we should try to merge both code someday.
@@ -711,7 +712,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
// representation. See CASSANDRA-11629
if (column.type.isCounter())
{
- ByteBuffer foundValue = getValue(metadata,
partitionKey, row);
+ ByteBuffer foundValue = getValue(metadata,
partitionKey, row, nowInSec);
if (foundValue == null)
return false;
@@ -721,7 +722,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
else
{
// Note that CQL expression are always of the form 'x <
4', i.e. the tested value is on the left.
- ByteBuffer foundValue = getValue(metadata,
partitionKey, row);
+ ByteBuffer foundValue = getValue(metadata,
partitionKey, row, nowInSec);
return foundValue != null &&
operator.isSatisfiedBy(column.type, foundValue, value);
}
}
@@ -736,7 +737,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
}
else
{
- ByteBuffer foundValue = getValue(metadata,
partitionKey, row);
+ ByteBuffer foundValue = getValue(metadata,
partitionKey, row, nowInSec);
return foundValue != null &&
operator.isSatisfiedBy(column.type, foundValue, value);
}
}
@@ -821,7 +822,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
return CompositeType.build(ByteBufferAccessor.instance, key,
value);
}
- public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row)
+ public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row, long nowInSec)
{
assert key != null;
// We support null conditions for LWT (in ColumnCondition) but
not for RowFilter.
@@ -839,7 +840,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
}
else
{
- ByteBuffer serializedMap = getValue(metadata, partitionKey,
row);
+ ByteBuffer serializedMap = getValue(metadata, partitionKey,
row, nowInSec);
if (serializedMap == null)
return false;
@@ -940,7 +941,7 @@ public class RowFilter implements
Iterable<RowFilter.Expression>
}
// Filtering by custom expressions isn't supported yet, so just
accept any row
- public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row)
+ public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row, long nowInSec)
{
return true;
}
diff --git a/src/java/org/apache/cassandra/db/rows/Cell.java
b/src/java/org/apache/cassandra/db/rows/Cell.java
index 3ddfeae39a..f7ecb5c8fb 100644
--- a/src/java/org/apache/cassandra/db/rows/Cell.java
+++ b/src/java/org/apache/cassandra/db/rows/Cell.java
@@ -240,6 +240,26 @@ public abstract class Cell<V> extends ColumnData
// timestamp on expiry.
}
+ /**
+ * Validates a cell's liveliness, tombstone status, and buffer contents.
+ *
+ * @param cell The cell to validate.
+ * @param nowInSeconds The current time in seconds.
+ * @return A ByteBuffer (including valid empty buffers) if valid, or
null otherwise.
+ */
+ public static ByteBuffer getValidCellBuffer(Cell<?> cell, long
nowInSeconds) {
+ if (cell == null || cell.isTombstone()) {
+ return null;
+ }
+
+ if (!cell.isLive(nowInSeconds)) {
+ return null;
+ }
+
+ // Allow valid empty buffers
+ return cell.buffer();
+ }
+
/**
* The serialization format for cell is:
* [ flags ][ timestamp ][ deletion time ][ ttl ][ path size
][ path ][ value size ][ value ]
diff --git
a/test/unit/org/apache/cassandra/index/sai/plan/OperationTest.java
b/test/unit/org/apache/cassandra/index/sai/plan/OperationTest.java
index 8b0acaaf2b..81292cbda0 100644
--- a/test/unit/org/apache/cassandra/index/sai/plan/OperationTest.java
+++ b/test/unit/org/apache/cassandra/index/sai/plan/OperationTest.java
@@ -515,7 +515,7 @@ public class OperationTest
}
@Override
- public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row)
+ public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row, long nowInSec)
{
throw new UnsupportedOperationException();
}
diff --git
a/test/unit/org/apache/cassandra/index/sasi/plan/OperationTest.java
b/test/unit/org/apache/cassandra/index/sasi/plan/OperationTest.java
index 79c86b977f..d8bc539c71 100644
--- a/test/unit/org/apache/cassandra/index/sasi/plan/OperationTest.java
+++ b/test/unit/org/apache/cassandra/index/sasi/plan/OperationTest.java
@@ -651,7 +651,7 @@ public class OperationTest extends SchemaLoader
}
@Override
- public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row)
+ public boolean isSatisfiedBy(TableMetadata metadata, DecoratedKey
partitionKey, Row row, long nowInSec)
{
throw new UnsupportedOperationException();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
