From f0280fb6f863582125bfd16b8a90c345972c4a8c Mon Sep 17 00:00:00 2001 From: Aleksey Yeschenko Date: Wed, 1 Sep 2021 12:22:31 -0400 Subject: [PATCH] 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 + .../cql3/statements/AlterTableStatement.java | 2 +- .../cassandra/db/marshal/AbstractType.java | 19 +++++++++++++-- .../cassandra/db/marshal/CollectionType.java | 11 ++++++++- .../cassandra/db/marshal/ReversedType.java | 6 ----- .../cql3/validation/operations/AlterTest.java | 24 +++++++++++++++++++ 6 files changed, 53 insertions(+), 10 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 702733eaaf..3383f14564 100644 --- a/CHANGES.txt +++ b/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) diff --git a/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java b/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java index 00d6fabad2..cd1041e6c1 100644 --- a/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java +++ b/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", diff --git a/src/java/org/apache/cassandra/db/marshal/AbstractType.java b/src/java/org/apache/cassandra/db/marshal/AbstractType.java index b14af70a4b..2bdc3c15c7 100644 --- a/src/java/org/apache/cassandra/db/marshal/AbstractType.java +++ b/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 implements Comparator * * 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 implements Comparator 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. * diff --git a/src/java/org/apache/cassandra/db/marshal/CollectionType.java b/src/java/org/apache/cassandra/db/marshal/CollectionType.java index d65e3a69e8..579c27b549 100644 --- a/src/java/org/apache/cassandra/db/marshal/CollectionType.java +++ b/src/java/org/apache/cassandra/db/marshal/CollectionType.java @@ -168,7 +168,7 @@ public abstract class CollectionType extends AbstractType 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 extends AbstractType 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); diff --git a/src/java/org/apache/cassandra/db/marshal/ReversedType.java b/src/java/org/apache/cassandra/db/marshal/ReversedType.java index 82a189533d..8ad5ef845b 100644 --- a/src/java/org/apache/cassandra/db/marshal/ReversedType.java +++ b/src/java/org/apache/cassandra/db/marshal/ReversedType.java @@ -108,12 +108,6 @@ public class ReversedType extends AbstractType return this.baseType.isCompatibleWith(((ReversedType) otherType).baseType); } - @Override - public boolean isValueCompatibleWith(AbstractType otherType) - { - return this.baseType.isValueCompatibleWith(otherType); - } - @Override public CQL3Type asCQL3Type() { diff --git a/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java b/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java index 3b5a5f7d00..d1f5206ff8 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java +++ b/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, PRIMARY KEY (k, c))"); + execute("alter table %s drop v"); + execute("alter table %s add v list"); + } + + @Test(expected = InvalidRequestException.class) + public void testDropSimpleAddComplex() throws Throwable + { + createTable("create table %s (k int, c int, v set, 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