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/24 17:25:23 UTC
[PR] [CASSANDRA-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
dcapwell opened a new pull request, #4006:
URL: https://github.com/apache/cassandra/pull/4006
(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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010631555
##########
src/java/org/apache/cassandra/cql3/Operation.java:
##########
@@ -374,8 +374,16 @@ public Operation prepare(TableMetadata metadata, ColumnMetadata receiver, boolea
{
if (!(receiver.type instanceof CollectionType))
{
- if (!(receiver.type instanceof CounterColumnType))
- throw new InvalidRequestException(String.format("Invalid operation (%s) for non counter column %s", toString(receiver), receiver.name));
+ if (canReadExistingState)
+ {
+ if (!(receiver.type instanceof NumberType<?>))
+ throw new InvalidRequestException(String.format("Invalid operation (%s) for non-numeric type %s", toString(receiver), receiver.name));
+ }
Review Comment:
new feature, `-=` now supports numeric types, just like `+=` does
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010668555
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -419,6 +427,7 @@ public class State extends CommonState
private final List<Symbol> searchableNonPartitionColumns;
private final List<Symbol> searchableColumns;
private final List<Symbol> nonPkIndexedColumns;
+ private final boolean isCas;
Review Comment:
i actually plan to make this a new test class that extends, rather than add CAS at the lower level. The main reason is mixing CAS with non CAS isn't deterministic
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010676914
##########
test/unit/org/apache/cassandra/cql3/ast/ReferenceExpression.java:
##########
@@ -26,6 +26,6 @@ public interface ReferenceExpression extends Expression
@Override
default ReferenceExpression visit(Visitor v)
{
- return this;
+ return v.visit(this);
Review Comment:
bug: wouldn't allow altering a reference!
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010648526
##########
src/java/org/apache/cassandra/cql3/terms/Constants.java:
##########
@@ -489,8 +489,10 @@ public void execute(DecoratedKey partitionKey, UpdateParameters params) throws I
else if (column.type instanceof NumberType<?>)
{
@SuppressWarnings("unchecked") NumberType<Number> type = (NumberType<Number>) column.type;
- ByteBuffer increment = t.bindAndGet(params.options);
- ByteBuffer current = getCurrentCellBuffer(partitionKey, params);
+ ByteBuffer increment = type.sanitize(t.bindAndGet(params.options));
Review Comment:
`type.sanitize` is a bug fix for empty bytes. Numeric types allow `byte[0]` and its semantic is weird... in 90% of the codebase its `null` but in that 10% its not... this code path hits a NPE as its part of the 90%...
empty is only not null if and only if you do `col = <empty>`, any other path its `null`... which includes this path now (else we NPE)
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010660820
##########
src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java:
##########
@@ -388,6 +388,7 @@ public void addConditions(Collection<ColumnCondition> conds, QueryOptions option
public boolean appliesTo(FilteredPartition current) throws InvalidRequestException
{
Row row = current.getRow(clustering);
+ if (row == null) return false;
Review Comment:
Benedict asked i revert this as it changes semantics...
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2019162752
##########
src/java/org/apache/cassandra/service/paxos/Paxos.java:
##########
@@ -1111,7 +1111,12 @@ private static BeginResult begin(long deadline,
PaxosPrepare.Success success = prepare.success();
Supplier<Participants> plan = () -> success.participants;
- DataResolver<?, ?> resolver = new DataResolver<>(query, plan, NoopReadRepair.instance, new Dispatcher.RequestTime(query.creationTimeNanos()));
+ long createdAtNanos = query.creationTimeNanos();
Review Comment:
pushed a change to pass the `Dispatcher.RequestTime` through. write side uses `CASRequest` and the read path just passes it 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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#issuecomment-2762691025
merged https://github.com/apache/cassandra/commit/b56edf2a5df8c320b33e38116f2742ab69f7b4fd
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2011017685
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -460,7 +466,8 @@ public State(RandomSource rs, Cluster cluster)
{
model.factory.regularAndStaticColumns.forEach(mutationGenBuilder::allowEmpty);
}
- this.mutationGen = toGen(mutationGenBuilder.build());
+ model.factory.regularAndStaticColumns.forEach(mutationGenBuilder::allowNull);
Review Comment:
was doing empty bytes and normal values, now injecting `null` as well.... this was to try to inline as much of `org.apache.cassandra.cql3.RandomSchemaTest` we could
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2012982233
##########
src/java/org/apache/cassandra/cql3/terms/Constants.java:
##########
@@ -499,7 +509,9 @@ else if (column.type instanceof NumberType<?>)
else if (column.type instanceof StringType)
{
ByteBuffer append = t.bindAndGet(params.options);
- ByteBuffer current = getCurrentCellBuffer(partitionKey, params);
+ if (append == null)
Review Comment:
would NPE with `+= null`
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2019144971
##########
src/java/org/apache/cassandra/db/marshal/AbstractType.java:
##########
@@ -537,6 +531,22 @@ public boolean allowsEmpty()
return false;
}
+ /**
+ * Returns {@code true} for types where empty should be handled like {@code null} like {@link Int32Type}.
+ */
+ public boolean isEmptyValueMeaningless()
Review Comment:
it was so the `null` and `empty` related logic were in the same place, right now they are in 2 different parts of this file
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2019120174
##########
src/java/org/apache/cassandra/service/paxos/Paxos.java:
##########
@@ -1111,7 +1111,12 @@ private static BeginResult begin(long deadline,
PaxosPrepare.Success success = prepare.success();
Supplier<Participants> plan = () -> success.participants;
- DataResolver<?, ?> resolver = new DataResolver<>(query, plan, NoopReadRepair.instance, new Dispatcher.RequestTime(query.creationTimeNanos()));
+ long createdAtNanos = query.creationTimeNanos();
Review Comment:
Is `query.creationTimeNanos` the wrong value? Shouldn't we plumb through `Dispatcher.RequestTime` and use it the way we would for normal reads by calling `requestTime.startedAtNanos()` and passing that to `setMonitoringTime`?
The real bug here is that it's not called at all for PaxosV2 right?
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/StatefulASTBase.java:
##########
@@ -333,11 +333,18 @@ protected ConsistencyLevel mutationCl()
{
var inst = selectInstance(rs);
String postfix = "on " + inst;
+ if (mutation.isCas())
+ {
+ postfix += ", would apply " + model.shouldApply(mutation);
+ // CAS doesn't allow timestamps
Review Comment:
CAS doesn't allow timestamps so neither does Accord using the CAS syntax. For non-CAS it does allow timestamps and in fact users coordinator generated timestamps not Accord timestamps at the moment for everything.
It would take a little more work to use Accord timestamps where possible with non-SERIAL.
##########
src/java/org/apache/cassandra/db/marshal/AbstractType.java:
##########
@@ -537,6 +531,22 @@ public boolean allowsEmpty()
return false;
}
+ /**
+ * Returns {@code true} for types where empty should be handled like {@code null} like {@link Int32Type}.
+ */
+ public boolean isEmptyValueMeaningless()
Review Comment:
Why is this moving? Unnecessary churn?
##########
test/unit/org/apache/cassandra/cql3/ast/AssignmentOperator.java:
##########
@@ -59,10 +59,15 @@ public static EnumSet<Kind> supportsOperators(AbstractType<?> type, boolean isTr
EnumSet<Kind> result = EnumSet.noneOf(Kind.class);
if (type instanceof CollectionType && type.isMultiCell())
{
- if (type instanceof SetType || type instanceof ListType)
+ if (type instanceof SetType)
return EnumSet.of(Kind.ADD, Kind.SUBTRACT);
+ if (type instanceof ListType)
Review Comment:
Is `-=` syntax even allowed with non-SERIAL? I thought it was rejected.
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010671385
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/StatefulASTBase.java:
##########
@@ -333,11 +333,18 @@ protected ConsistencyLevel mutationCl()
{
var inst = selectInstance(rs);
String postfix = "on " + inst;
+ if (mutation.isCas())
+ {
+ postfix += ", would apply " + model.shouldApply(mutation);
+ // CAS doesn't allow timestamps
Review Comment:
CAS actually rejects mutations, so had to add this logic.
I was under the impression that Accord Interop allows timestamps due to paxos allowing, but doesn't look like thats 100% true?
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010855240
##########
src/java/org/apache/cassandra/service/paxos/Paxos.java:
##########
@@ -1111,7 +1111,12 @@ private static BeginResult begin(long deadline,
PaxosPrepare.Success success = prepare.success();
Supplier<Participants> plan = () -> success.participants;
- DataResolver<?, ?> resolver = new DataResolver<>(query, plan, NoopReadRepair.instance, new Dispatcher.RequestTime(query.creationTimeNanos()));
+ long createdAtNanos = query.creationTimeNanos();
Review Comment:
bug found when enabling v2 testing. v2 would trigger ShortReadProtection which defines the timeout based off a `deadline` which we compute based off the value published to `Dispatcher.RequestTime`, the issue is that this request doesn't have its time set, so we get `-1` which breaks the timeout!
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010639883
##########
src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java:
##########
@@ -388,6 +388,7 @@ public void addConditions(Collection<ColumnCondition> conds, QueryOptions option
public boolean appliesTo(FilteredPartition current) throws InvalidRequestException
{
Row row = current.getRow(clustering);
+ if (row == null) return false;
Review Comment:
this is a partial bug fix... the bug found in the fuzz tests was as follows
```
v0 "someudt"
```
then the query
```
UPDATE ... WHERE pk=? IF v0 = ?
```
the issue here is that the partition didn't exist, so the LHS of the `ColumnCondition` and `Operator` were `null`. The condition was given empty bytes `byte[0]` (as the type allows it), and we have logic that says that a null LHS and an empty RHS is a matching condition... this caused us to *add* the partition!
This change here is trying to simplify this type of bug, but doesn't fully fix... You can't have a column condition return `true` if the row doesn't exist!
This does mean that the UDT bug still exists if `v0` isn't defined in the row... I think this is still a bug but need to help flesh out the semantics in dev@ to confirm this (empty bytes have undocumented and inconsistent symatnics...)
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010676271
##########
test/unit/org/apache/cassandra/cql3/ast/Mutation.java:
##########
@@ -404,9 +419,20 @@ public Statement visit(Visitor v)
Conditional copiedWhere = where.visit(v);
if (where != copiedWhere)
updated = true;
+ Optional<? extends CasCondition> updatedCasCondition = casCondition;
+ if (casCondition.isPresent())
Review Comment:
this was missed with the first round of `visit` impl. The side effect was mostly for the human readable `History:` dump when we find mismatches, as that would show a `?` rather than the CQL literal.
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2010648526
##########
src/java/org/apache/cassandra/cql3/terms/Constants.java:
##########
@@ -489,8 +489,10 @@ public void execute(DecoratedKey partitionKey, UpdateParameters params) throws I
else if (column.type instanceof NumberType<?>)
{
@SuppressWarnings("unchecked") NumberType<Number> type = (NumberType<Number>) column.type;
- ByteBuffer increment = t.bindAndGet(params.options);
- ByteBuffer current = getCurrentCellBuffer(partitionKey, params);
+ ByteBuffer increment = type.sanitize(t.bindAndGet(params.options));
Review Comment:
`type.sanitize` is a bug fix for empty bytes. Numeric types allow `byte[0]` and its semantic is weird... in 90% of the codebase its `null` but in that 10% its not... this code path hits a NPE as its part of the 90%...
empty is only not null if and only if you do `col = <empty>`, any other path its `null`...
##########
src/java/org/apache/cassandra/cql3/terms/Constants.java:
##########
@@ -524,19 +526,48 @@ public Substracter(ColumnMetadata column, Term t)
super(column, t);
}
+ @Override
+ public boolean requiresRead()
+ {
+ return !column.type.isCounter();
+ }
+
public void execute(DecoratedKey partitionKey, UpdateParameters params) throws InvalidRequestException
{
- ByteBuffer bytes = t.bindAndGet(params.options);
- if (bytes == null)
- throw new InvalidRequestException("Invalid null value for counter increment");
- if (bytes == ByteBufferUtil.UNSET_BYTE_BUFFER)
- return;
+ if (column.type instanceof CounterColumnType)
+ {
+ ByteBuffer bytes = t.bindAndGet(params.options);
+ if (bytes == null)
+ throw new InvalidRequestException("Invalid null value for counter increment");
+ if (bytes == ByteBufferUtil.UNSET_BYTE_BUFFER)
+ return;
- long increment = ByteBufferUtil.toLong(bytes);
- if (increment == Long.MIN_VALUE)
- throw new InvalidRequestException("The negation of " + increment + " overflows supported counter precision (signed 8 bytes integer)");
+ long increment = ByteBufferUtil.toLong(bytes);
+ if (increment == Long.MIN_VALUE)
+ throw new InvalidRequestException("The negation of " + increment + " overflows supported counter precision (signed 8 bytes integer)");
- params.addCounter(column, -increment);
+ params.addCounter(column, -increment);
+ }
+ else if (column.type instanceof NumberType<?>)
Review Comment:
new feature, `-=` now supports numeric types, just like `+=` does
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2019082390
##########
src/java/org/apache/cassandra/cql3/terms/Constants.java:
##########
@@ -499,7 +509,9 @@ else if (column.type instanceof NumberType<?>)
else if (column.type instanceof StringType)
{
ByteBuffer append = t.bindAndGet(params.options);
- ByteBuffer current = getCurrentCellBuffer(partitionKey, params);
+ if (append == null)
Review Comment:
Is `+=` null an error or should it silently succeed and do nothing? How do we get to appending `null`? I am not clear on what happens here.
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell closed pull request #4006: [CASSANDRA-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE
URL: https://github.com/apache/cassandra/pull/4006
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2015055658
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/MultiNodeTableWalkWithReadRepairTest.java:
##########
@@ -38,7 +38,9 @@ protected void preCheck(Cluster cluster, Property.StatefulBuilder builder)
// if a failing seed is detected, populate here
// Example: builder.withSeed(42L);
// CQL operations may have opertors such as +, -, and / (example 4 + 4), to "apply" them to get a constant value
-// CQL_DEBUG_APPLY_OPERATOR = true;
+ // CQL_DEBUG_APPLY_OPERATOR = true;
Review Comment:
just fixing comments to match the rest of the code examples
##########
test/distributed/org/apache/cassandra/distributed/test/cql3/MultiNodeTableWalkWithoutReadRepairTest.java:
##########
@@ -35,6 +35,8 @@ protected void preCheck(Cluster cluster, Property.StatefulBuilder builder)
// if a failing seed is detected, populate here
// Example: builder.withSeed(42L);
// CQL operations may have opertors such as +, -, and / (example 4 + 4), to "apply" them to get a constant value
-// CQL_DEBUG_APPLY_OPERATOR = true;
+ // CQL_DEBUG_APPLY_OPERATOR = true;
Review Comment:
just fixing comments to match the rest of the code examples
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2019082390
##########
src/java/org/apache/cassandra/cql3/terms/Constants.java:
##########
@@ -499,7 +509,9 @@ else if (column.type instanceof NumberType<?>)
else if (column.type instanceof StringType)
{
ByteBuffer append = t.bindAndGet(params.options);
- ByteBuffer current = getCurrentCellBuffer(partitionKey, params);
+ if (append == null)
Review Comment:
Is `+=` null an error or should it silently succeed and do nothing? How do we get to appending `null`? I am not clear on what happens here.
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2019144322
##########
test/unit/org/apache/cassandra/cql3/ast/AssignmentOperator.java:
##########
@@ -59,10 +59,15 @@ public static EnumSet<Kind> supportsOperators(AbstractType<?> type, boolean isTr
EnumSet<Kind> result = EnumSet.noneOf(Kind.class);
if (type instanceof CollectionType && type.isMultiCell())
{
- if (type instanceof SetType || type instanceof ListType)
+ if (type instanceof SetType)
return EnumSet.of(Kind.ADD, Kind.SUBTRACT);
+ if (type instanceof ListType)
Review Comment:
`-=` is allowed but only for List, this does a read before write so the consistency is a function of the CL you used for the write... this isn't safe but is the current behavior.
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2019171238
##########
test/unit/org/apache/cassandra/cql3/ast/AssignmentOperator.java:
##########
@@ -59,10 +59,15 @@ public static EnumSet<Kind> supportsOperators(AbstractType<?> type, boolean isTr
EnumSet<Kind> result = EnumSet.noneOf(Kind.class);
if (type instanceof CollectionType && type.isMultiCell())
{
- if (type instanceof SetType || type instanceof ListType)
+ if (type instanceof SetType)
return EnumSet.of(Kind.ADD, Kind.SUBTRACT);
+ if (type instanceof ListType)
Review Comment:
Huh, does the Accord translation of non-SERIAL reads handle this correctly by pushing it into the transaction? I don't think it does because this is solved for CAS only I think. I mean I guess it's non-SERIAL so it's fine for it not to be linearizable since we aren't committed to promising linearizability for non-SERIAL, but it will be a barrier to improving later.
--
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-20477](https://issues.apache.org/jira/browse/CASSANDRA-20477): Add support in CAS for -= on numeric types, and fixed improper handling of empty bytes which lead to NPE [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4006:
URL: https://github.com/apache/cassandra/pull/4006#discussion_r2017707724
##########
test/unit/org/apache/cassandra/cql3/ast/AssignmentOperator.java:
##########
@@ -59,10 +59,15 @@ public static EnumSet<Kind> supportsOperators(AbstractType<?> type, boolean isTr
EnumSet<Kind> result = EnumSet.noneOf(Kind.class);
if (type instanceof CollectionType && type.isMultiCell())
{
- if (type instanceof SetType || type instanceof ListType)
+ if (type instanceof SetType)
return EnumSet.of(Kind.ADD, Kind.SUBTRACT);
+ if (type instanceof ListType)
Review Comment:
List's `-=` does a read before write to know how to delete, and this does the read at the CL that the write is done at. In these tests multi node clusters tend to do `NODE_LOCAL` for writes and `ALL` for reads (more to stress reads and to lower the non-deterministic behavior). Given this, only transactional queries (CAS/Accord) are safe to add `-=`.
--
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