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 9 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.
| // 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...
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