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;
