Backport of CASSANDRA-16905 Further restrict schema column drop/recreate conversions

patch by Aleksey Yeschenko; reviewed by Blake Eggleston, Sam Tunnicliffe, and Caleb Rackliffe for CASSANDRA-16905

Co-authored by Aleksey Yeschenko (aleksey@apache.org)
Co-authored by Josh McKenzie (jmckenzie@apache.org)

CHANGES.txt

@@ -1,4 +1,5 @@
 3.0.30
+ * Backport of CASSANDRA-16905 Further restrict schema column drop/recreate conversions (CASSANDRA-18760)
  * CQLSH emits a warning when the server version doesn't match (CASSANDRA-18745)
  * Fix missing speculative retries in tablestats (CASSANDRA-18767)
  * Fix Requires for Java for RPM package (CASSANDRA-18751)

src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java

@@ -202,7 +202,7 @@ public class AlterTableStatement extends SchemaAlteringStatement
 
                     // After #8099, not safe to re-add columns of incompatible types - until *maybe* deser logic with dropped
                     // columns is pushed deeper down the line. The latter would still be problematic in cases of schema races.
-                    if (!type.isValueCompatibleWith(droppedColumn.type))
+                    if (!type.isSerializationCompatibleWith(droppedColumn.type))
                     {
                         String message =
                             String.format("Cannot re-add previously dropped column '%s' of type %s, incompatible with previous type %s",

src/java/org/apache/cassandra/db/marshal/AbstractType.java

@@ -34,6 +34,7 @@ import org.slf4j.LoggerFactory;
 import org.apache.cassandra.cql3.CQL3Type;
 import org.apache.cassandra.cql3.Term;
 import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.db.rows.Cell;
 import org.apache.cassandra.exceptions.SyntaxException;
 import org.apache.cassandra.serializers.TypeSerializer;
 import org.apache.cassandra.serializers.MarshalException;
@@ -279,9 +280,11 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>
      *
      * Note that a type should be compatible with at least itself.
      */
-    public boolean isValueCompatibleWith(AbstractType<?> otherType)
+    public boolean isValueCompatibleWith(AbstractType<?> previous)
     {
-        return isValueCompatibleWithInternal((otherType instanceof ReversedType) ? ((ReversedType) otherType).baseType : otherType);
+        AbstractType<?> thisType =          isReversed() ? ((ReversedType<?>)     this).baseType : this;
+        AbstractType<?> thatType = previous.isReversed() ? ((ReversedType<?>) previous).baseType : previous;
+        return thisType.isValueCompatibleWithInternal(thatType);
     }
 
     /**
@@ -293,6 +296,18 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer>
         return isCompatibleWith(otherType);
     }
 
+    /**
+     * Similar to {@link #isValueCompatibleWith(AbstractType)}, but takes into account {@link Cell} encoding.
+     * In particular, this method doesn't consider two types serialization compatible if one of them has fixed
+     * length (overrides {@link #valueLengthIfFixed()}, and the other one doesn't.
+     */
+    public boolean isSerializationCompatibleWith(AbstractType<?> previous)
+    {
+        return isValueCompatibleWith(previous)
+               && valueLengthIfFixed() == previous.valueLengthIfFixed()
+               && isMultiCell() == previous.isMultiCell();
+    }
+
     /**
      * An alternative comparison function used by CollectionsType in conjunction with CompositeType.
      *

src/java/org/apache/cassandra/db/marshal/CollectionType.java

@@ -168,7 +168,7 @@ public abstract class CollectionType<T> extends AbstractType<T>
             return false;
 
         // the value comparator is only used for Cell values, so sorting doesn't matter
-        return this.valueComparator().isValueCompatibleWith(tprev.valueComparator());
+        return this.valueComparator().isSerializationCompatibleWith(tprev.valueComparator());
     }
 
     @Override
@@ -192,6 +192,15 @@ public abstract class CollectionType<T> extends AbstractType<T>
         return isValueCompatibleWithFrozen(tprev);
     }
 
+    @Override
+    public boolean isSerializationCompatibleWith(AbstractType<?> previous)
+    {
+        if (!isValueCompatibleWith(previous))
+            return false;
+
+        return valueComparator().isSerializationCompatibleWith(((CollectionType<?>)previous).valueComparator());
+    }
+
     /** A version of isCompatibleWith() to deal with non-multicell (frozen) collections */
     protected abstract boolean isCompatibleWithFrozen(CollectionType<?> previous);
 

src/java/org/apache/cassandra/db/marshal/ReversedType.java

@@ -108,12 +108,6 @@ public class ReversedType<T> extends AbstractType<T>
         return this.baseType.isCompatibleWith(((ReversedType) otherType).baseType);
     }
 
-    @Override
-    public boolean isValueCompatibleWith(AbstractType<?> otherType)
-    {
-        return this.baseType.isValueCompatibleWith(otherType);
-    }
-
     @Override
     public CQL3Type asCQL3Type()
     {

test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java

@@ -338,6 +338,30 @@ public class AlterTest extends CQLTester
         execute("ALTER TABLE %s RENAME c1 TO \"\"");
     }
 
+    @Test(expected = InvalidRequestException.class)
+    public void testDropFixedAddVariable() throws Throwable
+    {
+        createTable("create table %s (k int, c int, v int, PRIMARY KEY (k, c))");
+        execute("alter table %s drop v");
+        execute("alter table %s add v varint");
+    }
+
+    @Test(expected = InvalidRequestException.class)
+    public void testDropFixedCollectionAddVariableCollection() throws Throwable
+    {
+        createTable("create table %s (k int, c int, v list<int>, PRIMARY KEY (k, c))");
+        execute("alter table %s drop v");
+        execute("alter table %s add v list<varint>");
+    }
+
+    @Test(expected = InvalidRequestException.class)
+    public void testDropSimpleAddComplex() throws Throwable
+    {
+        createTable("create table %s (k int, c int, v set<text>, PRIMARY KEY (k, c))");
+        execute("alter table %s drop v");
+        execute("alter table %s add v blob");
+    }
+
     @Test
     // tests CASSANDRA-9565
     public void testDoubleWith() throws Throwable