You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by sl...@apache.org on 2013/10/21 10:11:40 UTC
git commit: Fix altering column types
Updated Branches:
refs/heads/cassandra-1.2 df188cc8d -> 189a60728
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-1.2
Commit: 189a60728db1e01bfeaa664b41431701fd684f5f
Parents: df188cc
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Mon Oct 21 10:10:54 2013 +0200
Committer: Sylvain Lebresne <sy...@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;