Updated Branches:
  refs/heads/cassandra-2.0 edd1226fd -> 5c5426233

Fix altering column types

patch by slebresne; reviewed by iamaleksey for CASSANDRA-6185


Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/189a6072
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/189a6072
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/189a6072

Branch: refs/heads/cassandra-2.0
Commit: 189a60728db1e01bfeaa664b41431701fd684f5f
Parents: df188cc
Author: Sylvain Lebresne <sylv...@datastax.com>
Authored: Mon Oct 21 10:10:54 2013 +0200
Committer: Sylvain Lebresne <sylv...@datastax.com>
Committed: Mon Oct 21 10:10:54 2013 +0200

----------------------------------------------------------------------
 CHANGES.txt                                     |  1 +
 .../cassandra/config/ColumnDefinition.java      |  3 +-
 .../cql3/statements/AlterTableStatement.java    | 35 ++++++++++++++++----
 .../cassandra/db/marshal/AbstractType.java      | 14 +++++++-
 .../apache/cassandra/db/marshal/BytesType.java  |  7 ++++
 .../cassandra/db/marshal/CompositeType.java     | 24 ++++++++++++++
 6 files changed, 76 insertions(+), 8 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/189a6072/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index 70bb919..117a200 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,5 +1,6 @@
 1.2.12
  * (Hadoop) Require CFRR batchSize to be at least 2 (CASSANDRA-6114)
+ * Fix altering column types (CASSANDRA-6185)
 
 
 1.2.11

http://git-wip-us.apache.org/repos/asf/cassandra/blob/189a6072/src/java/org/apache/cassandra/config/ColumnDefinition.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/config/ColumnDefinition.java 
b/src/java/org/apache/cassandra/config/ColumnDefinition.java
index db5f7ed..807f008 100644
--- a/src/java/org/apache/cassandra/config/ColumnDefinition.java
+++ b/src/java/org/apache/cassandra/config/ColumnDefinition.java
@@ -180,8 +180,9 @@ public class ColumnDefinition
         if (getIndexType() != null && def.getIndexType() != null)
         {
             // If an index is set (and not drop by this update), the validator 
shouldn't be change to a non-compatible one
+            // (and we want true comparator compatibility, not just value one, 
since the validator is used by LocalPartitioner to order index rows)
             if (!def.getValidator().isCompatibleWith(getValidator()))
-                throw new ConfigurationException(String.format("Cannot modify 
validator to a non-compatible one for column %s since an index is set", 
comparator.getString(name)));
+                throw new ConfigurationException(String.format("Cannot modify 
validator to a non-order-compatible one for column %s since an index is set", 
comparator.getString(name)));
 
             assert getIndexName() != null;
             if (!getIndexName().equals(def.getIndexName()))

http://git-wip-us.apache.org/repos/asf/cassandra/blob/189a6072/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java
----------------------------------------------------------------------
diff --git 
a/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java 
b/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java
index a247a4d..36ec56d 100644
--- a/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java
+++ b/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java
@@ -134,24 +134,45 @@ public class AlterTableStatement extends 
SchemaAlteringStatement
                             throw new 
InvalidRequestException(String.format("counter type is not supported for 
PRIMARY KEY part %s", columnName));
                         if (cfDef.hasCompositeKey)
                         {
-                            List<AbstractType<?>> newTypes = new 
ArrayList<AbstractType<?>>(((CompositeType) cfm.getKeyValidator()).types);
+                            List<AbstractType<?>> oldTypes = ((CompositeType) 
cfm.getKeyValidator()).types;
+                            if 
(!newType.isValueCompatibleWith(oldTypes.get(name.position)))
+                                throw new 
ConfigurationException(String.format("Cannot change %s from type %s to type %s: 
types are incompatible.",
+                                                                               
columnName,
+                                                                               
oldTypes.get(name.position).asCQL3Type(),
+                                                                               
validator));
+
+                            List<AbstractType<?>> newTypes = new 
ArrayList<AbstractType<?>>(oldTypes);
                             newTypes.set(name.position, newType);
                             
cfm.keyValidator(CompositeType.getInstance(newTypes));
                         }
                         else
                         {
+                            if 
(!newType.isValueCompatibleWith(cfm.getKeyValidator()))
+                                throw new 
ConfigurationException(String.format("Cannot change %s from type %s to type %s: 
types are incompatible.",
+                                                                               
columnName,
+                                                                               
cfm.getKeyValidator().asCQL3Type(),
+                                                                               
validator));
                             cfm.keyValidator(newType);
                         }
                         break;
                     case COLUMN_ALIAS:
                         assert cfDef.isComposite;
-                        List<AbstractType<?>> newTypes = new 
ArrayList<AbstractType<?>>(((CompositeType) cfm.comparator).types);
+                        List<AbstractType<?>> oldTypes = ((CompositeType) 
cfm.comparator).types;
+                        // Note that CFMetaData.validateCompatibility already 
validate the change we're about to do. However, the error message it
+                        // sends is a bit cryptic for a CQL3 user, so 
validating here for a sake of returning a better error message
+                        // Do note that we need isCompatibleWith here, not 
just isValueCompatibleWith.
+                        if 
(!validator.getType().isCompatibleWith(oldTypes.get(name.position)))
+                            throw new 
ConfigurationException(String.format("Cannot change %s from type %s to type %s: 
types are not order-compatible.",
+                                                                           
columnName,
+                                                                           
oldTypes.get(name.position).asCQL3Type(),
+                                                                           
validator));
+                        List<AbstractType<?>> newTypes = new 
ArrayList<AbstractType<?>>(oldTypes);
                         newTypes.set(name.position, validator.getType());
                         cfm.comparator = CompositeType.getInstance(newTypes);
                         break;
                     case VALUE_ALIAS:
                         // See below
-                        if 
(!validator.getType().isCompatibleWith(cfm.getDefaultValidator()))
+                        if 
(!validator.getType().isValueCompatibleWith(cfm.getDefaultValidator()))
                             throw new 
ConfigurationException(String.format("Cannot change %s from type %s to type %s: 
types are incompatible.",
                                                                            
columnName,
                                                                            
cfm.getDefaultValidator().asCQL3Type(),
@@ -160,10 +181,12 @@ public class AlterTableStatement extends 
SchemaAlteringStatement
                         break;
                     case COLUMN_METADATA:
                         ColumnDefinition column = 
cfm.getColumnDefinition(columnName.key);
-                        // Thrift allows to change a column validator so 
CFMetaData.validateCompatility will let it slide
+                        // Thrift allows to change a column validator so 
CFMetaData.validateCompatibility will let it slide
                         // if we change to an incompatible type (contrarily to 
the comparator case). But we don't want to
-                        // allow it for CQL3 (see #5882) so validating it 
explicitly here
-                        if 
(!validator.getType().isCompatibleWith(column.getValidator()))
+                        // allow it for CQL3 (see #5882) so validating it 
explicitly here. We only care about value compatibility
+                        // though since we won't compare values (except when 
there is an index, but that is validated by
+                        // ColumnDefinition already).
+                        if 
(!validator.getType().isValueCompatibleWith(column.getValidator()))
                             throw new 
ConfigurationException(String.format("Cannot change %s from type %s to type %s: 
types are incompatible.",
                                                                            
columnName,
                                                                            
column.getValidator().asCQL3Type(),

http://git-wip-us.apache.org/repos/asf/cassandra/blob/189a6072/src/java/org/apache/cassandra/db/marshal/AbstractType.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/marshal/AbstractType.java 
b/src/java/org/apache/cassandra/db/marshal/AbstractType.java
index cbba89c..140ea7f 100644
--- a/src/java/org/apache/cassandra/db/marshal/AbstractType.java
+++ b/src/java/org/apache/cassandra/db/marshal/AbstractType.java
@@ -213,7 +213,19 @@ public abstract class AbstractType<T> implements 
Comparator<ByteBuffer>
      */
     public boolean isCompatibleWith(AbstractType<?> previous)
     {
-        return this == previous;
+        return this.equals(previous);
+    }
+
+    /**
+     * Returns true if values of the previous AbstracType can be read by the 
this
+     * AbsractType. Note that this is a weaker version of isCompatibleWith, as 
it
+     * does not require that both type compare values the same way.
+     *
+     * Note that a type should be compatible with at least itself.
+     */
+    public boolean isValueCompatibleWith(AbstractType<?> previous)
+    {
+        return isCompatibleWith(previous);
     }
 
     /**

http://git-wip-us.apache.org/repos/asf/cassandra/blob/189a6072/src/java/org/apache/cassandra/db/marshal/BytesType.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/marshal/BytesType.java 
b/src/java/org/apache/cassandra/db/marshal/BytesType.java
index 1bb2bd2..a9fee0a 100644
--- a/src/java/org/apache/cassandra/db/marshal/BytesType.java
+++ b/src/java/org/apache/cassandra/db/marshal/BytesType.java
@@ -83,6 +83,13 @@ public class BytesType extends AbstractType<ByteBuffer>
         return this == previous || previous == AsciiType.instance || previous 
== UTF8Type.instance;
     }
 
+    @Override
+    public boolean isValueCompatibleWith(AbstractType<?> previous)
+    {
+        // BytesType can read anything
+        return true;
+    }
+
     public CQL3Type asCQL3Type()
     {
         return CQL3Type.Native.BLOB;

http://git-wip-us.apache.org/repos/asf/cassandra/blob/189a6072/src/java/org/apache/cassandra/db/marshal/CompositeType.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/marshal/CompositeType.java 
b/src/java/org/apache/cassandra/db/marshal/CompositeType.java
index 2a27617..8cb1e34 100644
--- a/src/java/org/apache/cassandra/db/marshal/CompositeType.java
+++ b/src/java/org/apache/cassandra/db/marshal/CompositeType.java
@@ -172,6 +172,30 @@ public class CompositeType extends AbstractCompositeType
         return true;
     }
 
+    @Override
+    public boolean isValueCompatibleWith(AbstractType<?> previous)
+    {
+        if (this == previous)
+            return true;
+
+        if (!(previous instanceof CompositeType))
+            return false;
+
+        // Extending with new components is fine
+        CompositeType cp = (CompositeType)previous;
+        if (types.size() < cp.types.size())
+            return false;
+
+        for (int i = 0; i < cp.types.size(); i++)
+        {
+            AbstractType tprev = cp.types.get(i);
+            AbstractType tnew = types.get(i);
+            if (!tnew.isValueCompatibleWith(tprev))
+                return false;
+        }
+        return true;
+    }
+
     private static class StaticParsedComparator implements ParsedComparator
     {
         final AbstractType<?> type;

Reply via email to