cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From slebre...@apache.org
Subject [1/3] git commit: Fix altering column types
Date Mon, 21 Oct 2013 08:17:05 GMT
Updated Branches:
  refs/heads/trunk d1f8c6f7a -> 477191b27


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/trunk
Commit: 189a60728db1e01bfeaa664b41431701fd684f5f
Parents: df188cc
Author: Sylvain Lebresne <sylvain@datastax.com>
Authored: Mon Oct 21 10:10:54 2013 +0200
Committer: Sylvain Lebresne <sylvain@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;


Mime
View raw message