You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dcapwell (via GitHub)" <gi...@apache.org> on 2025/03/31 23:21:56 UTC
[PR] CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests [cassandra]
dcapwell opened a new pull request, #4024:
URL: https://github.com/apache/cassandra/pull/4024
(no comment)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4024:
URL: https://github.com/apache/cassandra/pull/4024#discussion_r2025356027
##########
test/harry/main/org/apache/cassandra/harry/model/BytesPartitionState.java:
##########
@@ -53,50 +66,118 @@ public class BytesPartitionState
public final Clustering<ByteBuffer> key;
private final Token token;
private final PartitionState state;
+ private final Map<Symbol, MultiCell> staticMultiCell;
+ @Nullable
+ private final Map<Long, Map<Symbol, MultiCell>> rowMultiCell;
Review Comment:
base harry supports cell resolution for non-mutli cell columns but doesn't support multi cell. Spoke with Alex and said he didn't need this PR to push this logic into base harry, so leaving here for the moment...
I do think this "could" be unified better... having a concept of `Cell` in the API would let me push this logic to the executor layer rather than have this lagic also handle execution... that is a larger refactor and something to think through.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #4024:
URL: https://github.com/apache/cassandra/pull/4024#issuecomment-3109787674
putting this in the backlog as batch is very fuggly... plan to focus on different AST tests first
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4024:
URL: https://github.com/apache/cassandra/pull/4024#discussion_r2023771495
##########
src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java:
##########
@@ -149,7 +149,7 @@ public void addConditions(Clustering<?> clustering, Collection<ColumnCondition>
}
else if (!(condition instanceof ColumnsConditions))
{
- throw new InvalidRequestException("Cannot mix IF conditions and IF NOT EXISTS for the same row");
+ throw new InvalidRequestException("Cannot mix IF conditions and " + ((ToCQL) condition).toCQL() + " for the same row");
Review Comment:
this was an error msg bug, `IF EXISTS` was what the user provided but we told them `IF NOT EXISTS`.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4024:
URL: https://github.com/apache/cassandra/pull/4024#discussion_r2025165752
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -341,61 +380,142 @@ else if (!clusterings.isEmpty())
}
}
+ private static void maybeUpdateColumns(Set<Symbol> columns,
+ @Nullable BytesPartitionState.Row row,
+ long nowTs, Map<Symbol, Expression> set,
+ ColumnUpdate update)
+ {
+ if (columns.isEmpty())
+ {
+ update.update(nowTs, Collections.emptyMap());
+ return;
+ }
+ // static columns to add in. If we are doing something like += to a row that doesn't exist, we still update statics...
+ Map<Symbol, Update> write = new HashMap<>();
+ for (Symbol col : columns)
+ {
+ ByteBuffer current = row == null ? null : row.get(col);
+ Update result = eval(col, current, set.get(col));
+ if (result.kind == Update.Kind.SKIP) continue;
+ write.put(col, result);
+ }
+ if (!write.isEmpty())
+ update.update(nowTs, write);
+ }
+
+ public boolean shouldApply(Batch batch)
+ {
+ if (!batch.isCas()) return true;
+ //TODO (correctness): what happens with CAS on multiple partitions?
+ boolean shouldApply = batch.mutations.stream().allMatch(this::shouldApply);
+ if (shouldApply
+ && ignoredIssues.contains(KnownIssue.BATCH_CAS_DELETE_STATIC_WITH_UPDATE_IF_NOT_EXISTS)
+ && !factory.staticColumns.isEmpty())
+ {
+ // No really, should it?
+ // Let's say you have 2 mutations
+ // DELETE s0 FROM tbl WHERE pk=0 IF EXISTS
+ // INSERT INTO tbl (pk, ck, r) VALUES (0, 0, 0) IF NOT EXISTS
+ // There are 2 cases this query can be in: static columns have values, static columns are null
+ // When the static columns are not null, then this applies as expected, but when the static columns are null then the CAS read query returns empty!
+ // CAS does "SELECT s0, r FROM tbl WHERE pk=0 AND ck=0" and since the row and the static columns are null, the empty result is generated; so the DELETE's condition of "IF EXISTS" can not apply!
+ // Now, it doesn't mean that the partition doesn't exist, the DELETE outside a batch would do "SELECT s0 FROM tbl WHERE pk=0 LIMIT 1"
+ boolean partitionExistsButCasConditionWillFail = false;
+ boolean rowDoesNotExist = false;
+ for (var m : batch.mutations)
+ {
+ if (m.casCondition().isEmpty()) continue;
+ var condition = m.casCondition().get();
+
+ var row = referenceRow(m);
+ if (m.kind == Mutation.Kind.DELETE)
+ {
+ var delete = (Mutation.Delete) m;
+ if (!delete.columns.isEmpty()
+ && row.clustering == null // delete is partition level, so deletes static columns
+ && existsOrColumnConditionsEqNull(condition))
+ {
+ // are the columns deleted currently null?
+ var partition = partitions.get(row.partition);
+ if (partition != null)
+ {
+ if (partition.staticRow().isEmpty())
+ partitionExistsButCasConditionWillFail = true;
+ }
+ }
+ }
+ if (row.clustering != null && condition == CasCondition.Simple.NotExists)
+ rowDoesNotExist = true;
+ }
+ if (rowDoesNotExist && partitionExistsButCasConditionWillFail)
+ return false;
+ }
+ return shouldApply;
+ }
+
+ private static boolean existsOrColumnConditionsEqNull(CasCondition condition)
+ {
+ if (condition == CasCondition.Simple.Exists) return true;
+ var conditions = ((CasCondition.IfCondition) condition).conditional.simplify();
+ return conditions.stream().allMatch(c -> {
+ if (!(c instanceof Conditional.Where)) return false;
+ var where = (Conditional.Where) c;
+ if (where.kind != Inequality.EQUAL) return false;
+ return ExpressionEvaluator.eval(where.rhs) == null;
+ });
+ }
+
public boolean shouldApply(Mutation mutation)
{
if (!mutation.isCas()) return true;
return shouldApply(mutation, selectPartitionForCAS(mutation));
}
- private SelectResult selectPartitionForCAS(Mutation mutation)
+ private CasContext selectPartitionForCAS(Mutation mutation)
{
- var partition = partitions.get(factory.createRef(pd(mutation)));
- if (partition == null) return SelectResult.ordered(factory.selectionOrder, NO_ROWS);
-
- var cd = cdOrNull(mutation);
- var row = cd == null ? null : partition.get(cd);
- ImmutableUniqueList<Symbol> columns = cd != null ? factory.selectionOrder : factory.partitionAndStaticColumns;
- return SelectResult.ordered(columns, new ByteBuffer[][] { getRowAsByteBuffer(columns, partition, row)});
+ BytesPartitionState.Ref ref = referencePartition(mutation);
+ Clustering<ByteBuffer> cd = cdOrNull(mutation);
+ BytesPartitionState partition = partitions.get(ref);
+ return new CasContext(ref, cd, partition);
}
- private boolean shouldApply(Mutation mutation, SelectResult current)
+ private boolean shouldApply(Mutation mutation, CasContext ctx)
{
Preconditions.checkArgument(mutation.isCas());
// process condition
- CasCondition condition;
- switch (mutation.kind)
- {
- case INSERT:
- condition = CasCondition.Simple.NotExists;
- break;
- case UPDATE:
- condition = ((Mutation.Update) mutation).casCondition.get();
- break;
- case DELETE:
- condition = ((Mutation.Delete) mutation).casCondition.get();
- break;
- default:
- throw new UnsupportedOperationException(mutation.kind.name());
- }
+ CasCondition condition = mutation.casCondition().get();
+ boolean partitionOrRow = ctx.clustering == null;
+ boolean partitionKnown = ctx.partition != null;
+ BytesPartitionState.Row row = partitionKnown && !partitionOrRow
+ ? ctx.partition.get(ctx.clustering)
+ : null;
if (condition instanceof CasCondition.Simple)
{
- boolean hasPartition = current.rows.length > 0;
- boolean partitionOrRow = current.columns.equals(factory.partitionAndStaticColumns);
- boolean hasRow = partitionOrRow ? hasPartition : current.isAllDefined(factory.clusteringColumns);
+ if (partitionOrRow && factory.staticColumns.isEmpty())
+ throw new AssertionError("Attempted to create a EXISTS condition on partition without static columns; " + mutation.toCQL());
+ // CAS's definition of partition EXISTS isn't based off the partition existing, its based off the static row
+ // existing (aka at least 1 static column exists and is not null).
+ boolean hasPartition = partitionKnown && !ctx.partition.staticRow().isEmpty();
+ boolean hasRow = row != null && !row.isEmpty();
var simple = (CasCondition.Simple) condition;
switch (simple)
{
case Exists:
- return hasRow;
+ return partitionOrRow ? hasPartition : hasRow;
Review Comment:
turns out that CAS defines partition `EXISTS` as
1) partition is defined
2) static row is not empty
The previous logic defined it only as #1 so had to make this change to support the empty static row check
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell closed pull request #4024: CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests
URL: https://github.com/apache/cassandra/pull/4024
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4024:
URL: https://github.com/apache/cassandra/pull/4024#discussion_r2021904227
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/StatefulASTBase.java:
##########
@@ -504,7 +537,10 @@ private String humanReadable(Statement stmt, @Nullable String annotate)
{
// With UTF-8 some chars can cause printing issues leading to error messages that don't reproduce the original issue.
// To avoid this problem, always escape the CQL so nothing gets lost
- String cql = StringUtils.escapeControlChars(stmt.visit(debug).toCQL(CQLFormatter.None.instance));
+ CQLFormatter formatter = CQLFormatter.None.instance;
+ if (stmt.kind() == Statement.Kind.BATCH) // its really hard to read these without this...
+ formatter = new CQLFormatter.PrettyPrint();
Review Comment:
i kinda want a new one, the sub elements are not formatted but the batch is
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4024:
URL: https://github.com/apache/cassandra/pull/4024#discussion_r2021904840
##########
test/harry/main/org/apache/cassandra/harry/model/PartitionState.java:
##########
@@ -236,8 +236,16 @@ private RowState updateRowState(RowState currentState, IntFunction<Bijections.Bi
{
// Timestamp collision case
Bijections.Bijection<?> column = columns.apply(i);
- if (column.compare(vds[i], currentState.vds[i]) > 0)
+ // writing a null is the same as a tombstone, which has higher priority
+ if (vds[i] == MagicConstants.NIL_DESCR)
Review Comment:
TODO: merge this into one `if`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4024:
URL: https://github.com/apache/cassandra/pull/4024#discussion_r2021903174
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -366,7 +367,8 @@ public void test() throws IOException
Property.StatefulBuilder statefulBuilder = stateful().withExamples(10).withSteps(400);
preCheck(cluster, statefulBuilder);
statefulBuilder.check(commands(() -> rs -> createState(rs, cluster))
- .add(StatefulASTBase::insert)
+// .add(StatefulASTBase::insert)
Review Comment:
this is only done for my testing of `batch`, this won't get committed
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org