Skip to content
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.

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

CASSANDRA-20503 Add BATCH support for AST Harry and update existing fuzz tests #4024

Open
wants to merge 27 commits into
base: trunk
Choose a base branch
from

Conversation

dcapwell
Copy link
Contributor

@dcapwell dcapwell commented Mar 31, 2025

this patch got too big so split part of it out into #4044.

#4044 focuses on the CAS related changes and the handling of timestamp. Multi cell conflicts are left for this patch

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();
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 kinda want a new one, the sub elements are not formatted but the batch is

@@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: merge this into one if

@@ -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");
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 was an error msg bug, IF EXISTS was what the user provided but we told them IF NOT EXISTS.

var simple = (CasCondition.Simple) condition;
switch (simple)
{
case Exists:
return hasRow;
return partitionOrRow ? hasPartition : hasRow;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines +69 to +71
private final Map<Symbol, MultiCell> staticMultiCell;
@Nullable
private final Map<Long, Map<Symbol, MultiCell>> rowMultiCell;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant