lukasz-antoniak commented on code in PR #4431:
URL: https://github.com/apache/cassandra/pull/4431#discussion_r2605308444
##########
test/unit/org/apache/cassandra/db/marshal/AbstractTypeTest.java:
##########
@@ -960,17 +960,31 @@ public void testAssumedCompatibility()
@Test
public void testBackwardCompatibility()
{
+ // Fix for CASSANDRA-20979 altered behaviour of SimpleDateType and
TimeType by correctly
+ // returning the fix-length size in valueLengthIfFixed(). This change
impacted logic of
+ // isSerializationCompatibleWith(), which as a result complains about
backward compatibility of:
+ // 1) [SimpleDateType isSerializationCompatibleWith Int32Type, {}]
expected:<[fals]e> but was:<[tru]e>
+ // 2) [TimeType isSerializationCompatibleWith LongType, {}]
expected:<[fals]e> but was:<[tru]e>
+ // 3) [BytesType isSerializationCompatibleWith SimpleDateType, {}]
expected:<[tru]e> but was:<[fals]e>
+ // 4) [BytesType isSerializationCompatibleWith TimeType, {}]
expected:<[tru]e> but was:<[fals]e>
+ ImmutableMap<AbstractType, Set<AbstractType>> exclusions =
ImmutableMap.<AbstractType, Set<AbstractType>>builder()
+
.put(SimpleDateType.instance, ImmutableSet.of(Int32Type.instance))
+
.put(TimeType.instance, ImmutableSet.of(LongType.instance))
+
.put(BytesType.instance, ImmutableSet.of(SimpleDateType.instance,
TimeType.instance))
+
.build();
+
Review Comment:
That is a good catch and it triggered me to review complete fix for the
issue again.
I have tried changing the `isSerializationCompatibleWith` method as you
suggest, but then the test was failing:
```
[junit-timeout] 1) [BytesType isSerializationCompatibleWith TimeType, {}]
[junit-timeout] Expecting code not to raise a throwable but caught
[junit-timeout] "java.lang.AssertionError: Property falsified after 1
example(s)
[junit-timeout] Smallest found falsifying value(s) :-
[junit-timeout] 0
[junit-timeout] Cause was :-
[junit-timeout] org.junit.ComparisonFailure: [BytesType .deserialize
TimeType, {}] expected:<...yteBuffer[pos=0 lim=[8 cap=8]]> but
was:<...yteBuffer[pos=0 lim=[0 cap=0]]>
[junit-timeout] at
java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native
Method)
[junit-timeout] at
java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
[junit-timeout] at
java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
[junit-timeout] at
org.apache.cassandra.db.marshal.AbstractTypeTest.verifySerializationCompatibilityForSimpleCells(AbstractTypeTest.java:1285)
[junit-timeout] at
org.apache.cassandra.db.marshal.AbstractTypeTest.lambda$verifyTypesCompatibility$39(AbstractTypeTest.java:1132)
[junit-timeout] at
org.quicktheories.dsl.TheoryBuilder.lambda$checkAssert$7(TheoryBuilder.java:145)
[junit-timeout] at
org.quicktheories.impl.Property.tryFalsification(Property.java:23)
```
Updating `TimeType#valueLengthIfFixed()` to return concrete length, changes
deserialization behaviour of empty byte array to return the 8 element array
with zeros.
I have then tried to return to starting point and look at vector
implementation. The choice between fix-length and variable-length serialization
is done in two places - `VectorCodec` class and in `VectorType`:
```
public static <E> VectorCodec<E> of(VectorType type, TypeCodec<E>
subtypeCodec)
{
return subtypeCodec.isSerializedSizeFixed()
? new FixedLength<>(type, subtypeCodec)
: new VariableLength<>(type, subtypeCodec);
}
```
```
private VectorType(AbstractType<T> elementType, int dimension)
{
super(ComparisonType.CUSTOM);
// ...
this.serializer = elementType.isValueLengthFixed() ?
new FixedLengthSerializer() :
new VariableLengthSerializer();
}
```
The `TypeCodec#serializedSize()` is not matching
`AbstractType#valueLengthIfFixed()` for those two types. The [CI
run](https://pre-ci.cassandra.apache.org/job/cassandra/264/testReport) after
latest change looks good, but I need to compare the codec and type
implementations.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
