aweisberg commented on code in PR #4044:
URL: https://github.com/apache/cassandra/pull/4044#discussion_r2029396306
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -191,6 +209,157 @@ private void indexRowColumn(TreeMap<ByteBuffer,
List<PrimaryKey>> index, boolean
public void update(Mutation mutation)
{
if (!shouldApply(mutation)) return;
+ updateInternal(mutation);
+ }
+
+ public void updateAndValidate(ByteBuffer[][] actual, Mutation mutation)
Review Comment:
Seems like one `update` method with a nullable `actual` would work also and
expose complexity to the caller, but this is also fine.
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -219,25 +389,25 @@ private void update(Mutation.Insert insert)
Map<Symbol, Expression> values = insert.values;
if (!factory.staticColumns.isEmpty() &&
!Sets.intersection(factory.staticColumns.asSet(), values.keySet()).isEmpty())
{
- // static columns to add in. If we are doing something like += to
a row that doesn't exist, we still update statics...
- Map<Symbol, ByteBuffer> write = new HashMap<>();
- for (Symbol col : Sets.intersection(factory.staticColumns.asSet(),
values.keySet()))
- write.put(col, eval(values.get(col)));
- partition.setStaticColumns(write);
+
maybeUpdateColumns(Sets.intersection(factory.staticColumns.asSet(),
values.keySet()),
+ partition.staticRow(),
+ nowTs, values,
+ partition::setStaticColumns);
}
// table has clustering but non are in the write, so only pk/static
can be updated
if (!factory.clusteringColumns.isEmpty() &&
Sets.intersection(factory.clusteringColumns.asSet(), values.keySet()).isEmpty())
return;
- Map<Symbol, ByteBuffer> write = new HashMap<>();
- for (Symbol col : Sets.intersection(factory.regularColumns.asSet(),
values.keySet()))
- write.put(col, eval(values.get(col)));
- partition.setColumns(key(insert.values, factory.clusteringColumns),
- write,
- true);
+ BytesPartitionState finalPartition = partition;
+ var cd = key(insert.values, factory.clusteringColumns);
+ maybeUpdateColumns(Sets.intersection(factory.regularColumns.asSet(),
values.keySet()),
+ partition.get(cd),
+ nowTs, values,
+ (ts, write) -> finalPartition.setColumns(cd, ts,
write, true));
}
private void update(Mutation.Update update)
{
+ long nowTs = update.timestampOrDefault(numMutations);
Review Comment:
How can you get the timestamp for CAS? Isn't that determined by Paxos/Accord?
##########
test/unit/org/apache/cassandra/cql3/ast/Mutation.java:
##########
@@ -161,22 +166,38 @@ public Stream<? extends Element> stream()
{
return Stream.of(value);
}
+
+ public long get()
+ {
+ if (value.value() instanceof Long)
+ return (long) value.value();
+ return LongType.instance.compose(value.valueEncoded());
+ }
}
public static class Using implements Element
{
public final Optional<TTL> ttl;
public final Optional<Timestamp> timestamp;
- public Using(Optional<TTL> ttl, Optional<Timestamp> timestamp)
+ private Using(Optional<TTL> ttl, Optional<Timestamp> timestamp)
{
this.ttl = ttl;
this.timestamp = timestamp;
+ if (ttl.isEmpty() && timestamp.isEmpty())
Review Comment:
Does this break existing CQL in a way we care about? Was this already an
error somewhere else in C*?
##########
test/unit/org/apache/cassandra/cql3/ast/Select.java:
##########
@@ -479,17 +480,31 @@ public Builder table(TableMetadata table)
public static class TableBasedBuilder extends
BaseBuilder<TableBasedBuilder> implements
Conditional.ConditionalBuilderPlus<TableBasedBuilder>
{
private final TableMetadata metadata;
+ private final ImmutableUniqueList<Symbol> columns;
public TableBasedBuilder(TableMetadata metadata)
{
this.metadata = metadata;
source = Optional.of(TableReference.from(metadata));
+ var builder = ImmutableUniqueList.<Symbol>builder();
+ metadata.allColumnsInSelectOrder().forEachRemaining(c ->
builder.add(Symbol.from(c)));
+ columns = builder.buildAndClear();
}
@Override
public TableMetadata metadata()
{
return metadata;
}
+
+ private Symbol find(String name)
+ {
+ return columns.stream().filter(s ->
s.symbol.equals(name)).findAny().get();
+ }
+
+ public TableBasedBuilder columnSelection(String name)
Review Comment:
This is unused. Just an FYI any data structures we additionally build here
end up being created in production scenarios where prepared statements aren't
used so there is an incentive to not build ones that aren't absolutely needed.
##########
test/unit/org/apache/cassandra/cql3/ast/Mutation.java:
##########
@@ -678,6 +749,11 @@ public InsertBuilder ifNotExists()
return this;
}
+ public InsertBuilder timestamp(long value)
Review Comment:
Unused?
##########
test/unit/org/apache/cassandra/utils/ASTGenerators.java:
##########
@@ -391,6 +393,18 @@ public MutationGenBuilder(TableMetadata metadata)
columnExpressions.put(symbol, new
ExpressionBuilder(symbol.type()));
}
+ public MutationGenBuilder withAllowPartitionOnlyUpdate(boolean value)
+ {
+ this.allowPartitionOnlyUpdate = value;
+ return this;
+ }
+
+ public MutationGenBuilder withAllowPartitionOnlyInsert(boolean value)
Review Comment:
Unused?
##########
test/unit/org/apache/cassandra/utils/ASTGenerators.java:
##########
@@ -391,6 +393,18 @@ public MutationGenBuilder(TableMetadata metadata)
columnExpressions.put(symbol, new
ExpressionBuilder(symbol.type()));
}
+ public MutationGenBuilder withAllowPartitionOnlyUpdate(boolean value)
Review Comment:
Unused?
##########
test/unit/org/apache/cassandra/cql3/ast/Mutation.java:
##########
@@ -727,6 +803,11 @@ protected UpdateBuilder(TableMetadata table)
super(Kind.UPDATE, table);
}
+ public UpdateBuilder timestamp(long value)
Review Comment:
Also unused?
##########
test/unit/org/apache/cassandra/cql3/ast/Mutation.java:
##########
@@ -766,18 +847,32 @@ public UpdateBuilder set(Symbol column, Expression value)
public UpdateBuilder set(String column, int value)
{
- return set(new Symbol(column, Int32Type.instance), Bind.of(value));
+ Symbol symbol = find(column);
+ if (!symbol.type().equals(Int32Type.instance))
+ throw new AssertionError("Expected int type but given " +
symbol.type().asCQL3Type());
+ return set(symbol, Bind.of(value));
+ }
+
+ public UpdateBuilder set(String column, Object value)
+ {
+ Symbol symbol = find(column);
+ return set(symbol, new Bind(value, symbol.type()));
}
public UpdateBuilder set(String column, Expression expression)
{
- Symbol symbol = new Symbol(metadata.getColumn(new
ColumnIdentifier(column, true)));
- return set(symbol, expression);
+ return set(find(column), expression);
+ }
+
+ public UpdateBuilder set(String column, Function<Symbol, Expression>
fn)
Review Comment:
Also unused?
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -722,27 +939,9 @@ private static void
validateAnyOrder(ImmutableUniqueList<Symbol> columns, Set<Ro
var unexpected = Sets.difference(actual, expected);
var missing = Sets.difference(expected, actual);
StringBuilder sb = null;
Review Comment:
Why bother with a null string builder? Is it worth all the code to
initialize it later? Maybe do an empty check instead?
--
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-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
