CASSANDRA-21458 Update serializers to allow comparison between two LET variables#4879
CASSANDRA-21458 Update serializers to allow comparison between two LET variables#4879alanwang67 wants to merge 16 commits into
Conversation
| | '!=' { $op = ConditionStatement.Kind.NEQ; } | ||
| ; | ||
|
|
||
| txnConditionKindRef returns [ConditionStatement.Kind op] |
There was a problem hiding this comment.
isn't this a duplicate of
txnConditionKind returns [ConditionStatement.Kind op]
: '=' { $op = ConditionStatement.Kind.EQ; }
| '<' { $op = ConditionStatement.Kind.LT; }
| '<=' { $op = ConditionStatement.Kind.LTE; }
| '>' { $op = ConditionStatement.Kind.GT; }
| '>=' { $op = ConditionStatement.Kind.GTE; }
| '!=' { $op = ConditionStatement.Kind.NEQ; }
;
| LT(TxnCondition.Kind.LESS_THAN, TxnCondition.Kind.GREATER_THAN), | ||
| LTE(TxnCondition.Kind.LESS_THAN_OR_EQUAL, TxnCondition.Kind.GREATER_THAN_OR_EQUAL); | ||
|
|
||
| LTE(TxnCondition.Kind.LESS_THAN_OR_EQUAL, TxnCondition.Kind.GREATER_THAN_OR_EQUAL), |
There was a problem hiding this comment.
why are you duplicating this?
There was a problem hiding this comment.
Discussed offline, leaving here as documentation. This was done in order to allow for node upgrade compatibility. Since we decide which serializer to use based off the operator and ref op val and ref op ref, can not be differentiated by just the operator, the thinking was to create a new operator to differentiate the two. However, this resulted in duplicate code. The implementation now chooses to encode this information by using an extra bit within the TxnCondition.Kind ordinal that is serialized.
| throw new IllegalStateException(String.format("Row reference (%s) can only be used with IS NULL/IS NOT NULL conditions", lhs.getText())); | ||
| if (((RowDataReference.Raw) rhs).column() == null) | ||
| throw new IllegalStateException(String.format("Row reference (%s) can only be used with IS NULL/IS NOT NULL conditions", rhs.getText())); | ||
| if (!((RowDataReference.Raw) lhs).column().type.equals(((RowDataReference.Raw) rhs).column().type)) |
There was a problem hiding this comment.
I think strictly speaking this isn't required as int < long I believe we support; but its fine for now
There was a problem hiding this comment.
I was wondering if we allowed for comparisons between an int and a boolean today or would we raise a type error or more generically two types that don't make sense to be compared.
There was a problem hiding this comment.
I actually think IllegalStateException is the wrong type as we are validating the CQL, looking at TransactionStatement we use
package org.apache.cassandra.exceptions;
public class InvalidRequestException extends RequestValidationException
from
public static InvalidRequestException invalidRequest(String messageTemplate, Object... messageArgs)
{
return new InvalidRequestException(String.format(messageTemplate, messageArgs));
}
But back to the type thing. its fine to go specific type and if we ever have the time we can do castable types; I think its far more work than just undoing this if condition so fine to leave as is for now
| // This is so done to preserve upgrade compatibility with the prior serializer. | ||
| // Nodes that are not yet upgraded can still deserialize all values modulo those that are |
There was a problem hiding this comment.
In order for Accord to make changes to the serialization it needs to support V2 first which requires global configs, which is not in scope in the immediate future; given this we need work-arounds to support users who have been deploying off trunk to upgrade without breaking them; this is why we have the TOP_BIT concept, to enable this type of upgrade for users.
This change is safe under the following assumptions
1) `ref op ref` feature is only used after all nodes have been upgraded
2) cluster can be mixed mode as long as `ref op ref` is not used
If a user tries to use `ref op ref` in a mixed mode this will lead to undefined errors where the only recovery process is to force older nodes to upgrade
Something like that. Its very detailed on why this is the case...
| } | ||
|
|
||
| @Override | ||
| public boolean applies(TxnData data) |
There was a problem hiding this comment.
this isn't correct; there is a reason why Value doesn't do this as this is just SimpleBounds. row1.foo.bar and row1.foo['key'] are not correct I believe
There was a problem hiding this comment.
IF row1.user.id == row2.user.idthe above is actually the following in your code
IF row1.user == row2.useras the toByteBuffer takes all complex types and returns the top level type, and not what is actually referenced
| serializer.serialize(value, out); | ||
| } | ||
|
|
||
| public static <T, P> void serializeNullable(T value, P param, DataOutputPlus out, ParameterisedUnversionedSerializer<T, P> serializer) throws IOException |
There was a problem hiding this comment.
this is all dead code right? can we remove it if not used?
|
|
||
| cluster.coordinator(1).executeWithResult(insert, ConsistencyLevel.SERIAL); | ||
|
|
||
| String query = "BEGIN TRANSACTION\n" + |
There was a problem hiding this comment.
I pointed out in the code you have a bug with complex types. if you are doing a UDT field access we convert that to a UDT compare. If you are working with collection elements we convert that to a collection compare
| AbstractType<?> typeRHS = getColumnType(referenceRHS); | ||
|
|
||
| if (typeLHS != typeRHS) | ||
| throw new InvalidRequestException(String.format("Invalid type comparison: cannot compare type %s with type %s", typeLHS.asCQL3Type(), typeRHS.asCQL3Type())); |
There was a problem hiding this comment.
isn't this too late? won't you deadlock accord? We already accepted we can't reject anymore
2 participants
Update serializers to allow comparison between two LET variables
patch by Alan Wang;
reviewed by for CASSANDRA-21458
Co-authored-by: Name1
Co-authored-by: Name2