Add support for range deletes in AST fuzz testing #4222
Conversation
|
Sorry for neglecting this for so long. I’ll run this though CI to see what happens… if this doesn’t find bugs I’ll be shocked |
| @@ -517,6 +520,8 @@ private void update(Mutation.Delete delete) | |||
| DeleteKind kind = DeleteKind.PARTITION; | |||
| if (!delete.columns.isEmpty()) | |||
| kind = DeleteKind.COLUMN; | |||
| else if (containsRangeCondition(split.right)) | |||
There was a problem hiding this comment.
nit: if the range isn't on the clustering key, then its not a range tombstone... maybe rename to be clear this only matters for clustering?
| for (BytesPartitionState.Row value : partition.rows()) | ||
| { | ||
| if (ctx.include(value)) | ||
| partition.deleteRow(value.clustering, nowTs); |
There was a problem hiding this comment.
this could cause the partition to become empty, so we need to delete the partition in this case. Can you update the logic to handle that? There is similar logic w/e we delete rows
| { | ||
| for (Conditional cond : conditionals) | ||
| { | ||
| if (!(cond instanceof Conditional.Where)) |
| default: | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private boolean containsRangeCondition(List<Conditional> conditionals) |
There was a problem hiding this comment.
this is "correct" in the sense it answers the question about the existence of a range, but don't we only care about clustering columns in the only usage? The lang doesn't really support delete value by range but the AST does (it can test valid and not valid CQL).
| @@ -772,6 +805,7 @@ private Pair<List<Clustering<ByteBuffer>>, List<Conditional>> splitOn(ImmutableU | |||
| // pk requires equality | |||
| Map<Symbol, List<ByteBuffer>> pks = new HashMap<>(); | |||
| List<Conditional> other = new ArrayList<>(); | |||
| Map<Symbol, List<Conditional.Where>> rangeConditions = new HashMap<>(); | |||
There was a problem hiding this comment.
Why are you trying to handle ranges in this method? we call this on partition and if the partition has clustering ranges we then need to special case it and not call splitOn for clustering. I think we can drop the changes to this method
| for (Slice slice : slices) | ||
| { | ||
| ClusteringBound<?> start = slice.start(); | ||
| ClusteringBound<?> end = slice.end(); | ||
|
|
||
| if (!start.isBottom()) | ||
| clusterings.add(createClusteringBound(start)); | ||
|
|
||
| if (!end.isTop()) | ||
| clusterings.add(createClusteringBound(start)); | ||
| } | ||
| return Pair.create(clusterings, other); |
There was a problem hiding this comment.
this isn't correct... this method works with equality and not bounds, so returning bounds will yield incorrect responses. I think you can just drop all changes to this method as you have to special case range delete anyways before you call this method on clustering columns
| Conditional.Where.Inequality.LESS_THAN_EQ); | ||
|
|
||
|
|
||
| private void valueRange(RandomnessSource rnd, |
There was a problem hiding this comment.
there is a ton of duplication in the existing tests with regard to SELECT, could you see how painful it would be to make this a reusable builder that the tests can use?
org.apache.cassandra.distributed.test.cql3.SingleNodeTableWalkTest#simpleRangeSearch
| @Nullable Gen <? extends Map<Symbol, Object>> gen) | ||
| { | ||
| if (gen == null) | ||
| throw new UnsupportedOperationException("TODO: add support later... fine to ignore for now"); |
There was a problem hiding this comment.
Caused by: java.lang.UnsupportedOperationException: TODO: add support later... fine to ignore for now
at org.apache.cassandra.utils.ASTGenerators$MutationGenBuilder.valueRange(ASTGenerators.java:892)
at org.apache.cassandra.utils.ASTGenerators$MutationGenBuilder.lambda$build$11(ASTGenerators.java:795)
at org.apache.cassandra.utils.Generators.lambda$toGen$30(Generators.java:648)
at org.apache.cassandra.distributed.test.cql3.StatefulASTBase.insert(StatefulASTBase.java:212)
guess it isn't =)
org.apache.cassandra.distributed.test.cql3.FullAccordInteropMultiNodeTableWalkTest hit this
but assume that all the *TableWalkTest would
belliottsmith
force-pushed
the
trunk
branch
2 times, most recently
from
df3eb40 to
54e39a9
Compare
Cassandra-20634: Support for range deletes in AST testing framework.
Jira: https://issues.apache.org/jira/browse/CASSANDRA-20634