(CQL3) Allow definitions with only a PK patch by slebresne; reviewed by jbellis for CASSANDRA-4361
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/79517461 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/79517461 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/79517461 Branch: refs/heads/cassandra-1.1 Commit: 795174611b7a9bea4ee2d64f8640c8bfebe07cfb Parents: d0d1b2c Author: Sylvain Lebresne <sylv...@datastax.com> Authored: Tue Jul 24 09:31:56 2012 +0200 Committer: Sylvain Lebresne <sylv...@datastax.com> Committed: Tue Jul 24 09:31:56 2012 +0200 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../org/apache/cassandra/config/CFMetaData.java | 4 +- .../org/apache/cassandra/cql3/CFDefinition.java | 22 ++++-- .../cassandra/cql3/operations/ColumnOperation.java | 18 ++++- .../cassandra/cql3/statements/ColumnGroupMap.java | 6 -- .../statements/CreateColumnFamilyStatement.java | 66 +++++++++----- .../cassandra/cql3/statements/SelectStatement.java | 51 +++++++---- .../cassandra/cql3/statements/UpdateStatement.java | 43 ++++++++-- 8 files changed, 145 insertions(+), 66 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/79517461/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 8d61f64..979e3ef 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -30,6 +30,7 @@ * (cql3) Always use composite types by default (CASSANDRA-4329) * (cql3) Add support for set, map and list (CASSANDRA-3647) * Validate date type correctly (CASSANDRA-4441) + * (cql3) Allow definitions with only a PK (CASSANDRA-4361) 1.1.3 http://git-wip-us.apache.org/repos/asf/cassandra/blob/79517461/src/java/org/apache/cassandra/config/CFMetaData.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/config/CFMetaData.java b/src/java/org/apache/cassandra/config/CFMetaData.java index 3cf41d7..906017c 100644 --- a/src/java/org/apache/cassandra/config/CFMetaData.java +++ b/src/java/org/apache/cassandra/config/CFMetaData.java @@ -1080,8 +1080,6 @@ public final class CFMetaData { if (alias != null) { - if (!alias.hasRemaining()) - throw new ConfigurationException(msg + " alias may not be empty"); try { UTF8Type.instance.validate(alias); @@ -1259,9 +1257,9 @@ public final class CFMetaData cfm.caching(Caching.valueOf(result.getString("caching"))); cfm.compactionStrategyClass(createCompactionStrategy(result.getString("compaction_strategy_class"))); cfm.compressionParameters(CompressionParameters.create(fromJsonMap(result.getString("compression_parameters")))); + cfm.columnAliases(columnAliasesFromStrings(fromJsonList(result.getString("column_aliases")))); if (result.has("value_alias")) cfm.valueAlias(result.getBytes("value_alias")); - cfm.columnAliases(columnAliasesFromStrings(fromJsonList(result.getString("column_aliases")))); cfm.compactionStrategyOptions(fromJsonMap(result.getString("compaction_strategy_options"))); return cfm; http://git-wip-us.apache.org/repos/asf/cassandra/blob/79517461/src/java/org/apache/cassandra/cql3/CFDefinition.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/CFDefinition.java b/src/java/org/apache/cassandra/cql3/CFDefinition.java index 23edd62..d960a79 100644 --- a/src/java/org/apache/cassandra/cql3/CFDefinition.java +++ b/src/java/org/apache/cassandra/cql3/CFDefinition.java @@ -51,8 +51,8 @@ public class CFDefinition implements Iterable<CFDefinition.Name> public final Map<ColumnIdentifier, Name> metadata = new TreeMap<ColumnIdentifier, Name>(); public final boolean isComposite; - // Note that isCompact means here that no componet of the comparator correspond to the column names - // defined in the CREATE TABLE QUERY. This is not exactly equivalent to the 'WITH COMPACT STORAGE' + // Note that isCompact means here that no component of the comparator correspond to the column names + // defined in the CREATE TABLE QUERY. This is not exactly equivalent to using the 'WITH COMPACT STORAGE' // option when creating a table in that "static CF" without a composite type will have isCompact == false // even though one must use 'WITH COMPACT STORAGE' to declare them. public final boolean isCompact; @@ -66,7 +66,7 @@ public class CFDefinition implements Iterable<CFDefinition.Name> { this.isComposite = true; CompositeType composite = (CompositeType)cfm.comparator; - if (cfm.getColumn_metadata().isEmpty()) + if (cfm.getColumnAliases().size() == composite.types.size()) { // "dense" composite this.isCompact = true; @@ -76,7 +76,7 @@ public class CFDefinition implements Iterable<CFDefinition.Name> ColumnIdentifier id = getColumnId(cfm, i); this.columns.put(id, new Name(cfm.ksName, cfm.cfName, id, Name.Kind.COLUMN_ALIAS, i, composite.types.get(i))); } - this.value = new Name(cfm.ksName, cfm.cfName, getValueId(cfm), Name.Kind.VALUE_ALIAS, cfm.getDefaultValidator()); + this.value = createValue(cfm); } else { @@ -114,14 +114,14 @@ public class CFDefinition implements Iterable<CFDefinition.Name> { this.isComposite = false; this.hasCollections = false; - if (cfm.getColumn_metadata().isEmpty()) + if (!cfm.getColumnAliases().isEmpty() || cfm.getColumn_metadata().isEmpty()) { // dynamic CF this.isCompact = true; ColumnIdentifier id = getColumnId(cfm, 0); Name name = new Name(cfm.ksName, cfm.cfName, id, Name.Kind.COLUMN_ALIAS, 0, cfm.comparator); this.columns.put(id, name); - this.value = new Name(cfm.ksName, cfm.cfName, getValueId(cfm), Name.Kind.VALUE_ALIAS, cfm.getDefaultValidator()); + this.value = createValue(cfm); } else { @@ -171,6 +171,16 @@ public class CFDefinition implements Iterable<CFDefinition.Name> return (ColumnToCollectionType)composite.types.get(composite.types.size() - 1); } + private static Name createValue(CFMetaData cfm) + { + ColumnIdentifier alias = getValueId(cfm); + // That's how we distinguish between 'no value alias because coming from thrift' and 'I explicitely did not + // define a value' (see CreateColumnFamilyStatement) + return alias.key.equals(ByteBufferUtil.EMPTY_BYTE_BUFFER) + ? null + : new Name(cfm.ksName, cfm.cfName, alias, Name.Kind.VALUE_ALIAS, cfm.getDefaultValidator()); + } + public Name get(ColumnIdentifier name) { if (name.equals(key.name)) http://git-wip-us.apache.org/repos/asf/cassandra/blob/79517461/src/java/org/apache/cassandra/cql3/operations/ColumnOperation.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/operations/ColumnOperation.java b/src/java/org/apache/cassandra/cql3/operations/ColumnOperation.java index 5e4f6b3..4f36775 100644 --- a/src/java/org/apache/cassandra/cql3/operations/ColumnOperation.java +++ b/src/java/org/apache/cassandra/cql3/operations/ColumnOperation.java @@ -36,6 +36,17 @@ public class ColumnOperation implements Operation { enum Kind { SET, COUNTER_INC, COUNTER_DEC } + private static final Operation setToEmptyOperation = new ColumnOperation(null, Kind.SET) + { + @Override + protected void doSet(ColumnFamily cf, ColumnNameBuilder builder, AbstractType<?> validator, UpdateParameters params) throws InvalidRequestException + { + ByteBuffer colName = builder.build(); + QueryProcessor.validateColumnName(colName); + cf.addColumn(params.makeColumn(colName, ByteBufferUtil.EMPTY_BYTE_BUFFER)); + } + }; + private final Term value; private final Kind kind; @@ -69,7 +80,7 @@ public class ColumnOperation implements Operation throw new InvalidRequestException("Column operations are only supported on simple types, but " + validator + " given."); } - private void doSet(ColumnFamily cf, ColumnNameBuilder builder, AbstractType<?> validator, UpdateParameters params) throws InvalidRequestException + protected void doSet(ColumnFamily cf, ColumnNameBuilder builder, AbstractType<?> validator, UpdateParameters params) throws InvalidRequestException { ByteBuffer colName = builder.build(); QueryProcessor.validateColumnName(colName); @@ -133,4 +144,9 @@ public class ColumnOperation implements Operation { return new ColumnOperation(value, Kind.COUNTER_DEC); } + + public static Operation SetToEmpty() + { + return setToEmptyOperation; + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/79517461/src/java/org/apache/cassandra/cql3/statements/ColumnGroupMap.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/ColumnGroupMap.java b/src/java/org/apache/cassandra/cql3/statements/ColumnGroupMap.java index e365e81..2625fef 100644 --- a/src/java/org/apache/cassandra/cql3/statements/ColumnGroupMap.java +++ b/src/java/org/apache/cassandra/cql3/statements/ColumnGroupMap.java @@ -148,12 +148,6 @@ public class ColumnGroupMap */ private boolean isSameGroup(ByteBuffer[] c) { - // Cql don't allow to insert columns who doesn't have all component of - // the composite set for sparse composite. Someone coming from thrift - // could hit that though. But since we have no way to handle this - // correctly, better fail here and tell whomever may hit that (if - // someone ever do) to change the definition to a dense composite. - assert c.length >= idx && previous.length >= idx : "Sparse composite should not have partial column names"; for (int i = 0; i < idx; i++) { if (!c[i].equals(previous[i])) http://git-wip-us.apache.org/repos/asf/cassandra/blob/79517461/src/java/org/apache/cassandra/cql3/statements/CreateColumnFamilyStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/CreateColumnFamilyStatement.java b/src/java/org/apache/cassandra/cql3/statements/CreateColumnFamilyStatement.java index bf7a82a..f5466d6 100644 --- a/src/java/org/apache/cassandra/cql3/statements/CreateColumnFamilyStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/CreateColumnFamilyStatement.java @@ -44,6 +44,7 @@ import org.apache.cassandra.service.MigrationManager; import org.apache.cassandra.thrift.CqlResult; import org.apache.cassandra.thrift.InvalidRequestException; import org.apache.cassandra.io.compress.CompressionParameters; +import org.apache.cassandra.utils.ByteBufferUtil; import org.apache.cassandra.utils.Pair; /** A <code>CREATE COLUMNFAMILY</code> parsed from a CQL query statement. */ @@ -195,7 +196,26 @@ public class CreateColumnFamilyStatement extends SchemaAlteringStatement throw new InvalidRequestException(String.format("counter type is not supported for PRIMARY KEY part %s", stmt.keyAlias)); // Handle column aliases - if (!columnAliases.isEmpty()) + if (columnAliases.isEmpty()) + { + if (useCompactStorage) + { + // There should remain some column definition since it is a non-composite "static" CF + if (stmt.columns.isEmpty()) + throw new InvalidRequestException("No definition found that is not part of the PRIMARY KEY"); + + stmt.comparator = CFDefinition.definitionType; + } + else + { + List<AbstractType<?>> types = new ArrayList<AbstractType<?>>(definedCollections == null ? 1 : 2); + types.add(CFDefinition.definitionType); + if (definedCollections != null) + types.add(ColumnToCollectionType.getInstance(definedCollections)); + stmt.comparator = CompositeType.getInstance(types); + } + } + else { // If we use compact storage and have only one alias, it is a // standard "dynamic" CF, otherwise it's a composite @@ -240,42 +260,40 @@ public class CreateColumnFamilyStatement extends SchemaAlteringStatement stmt.comparator = CompositeType.getInstance(types); } } - else + + if (useCompactStorage && stmt.columns.size() <= 1) { - if (useCompactStorage) + if (stmt.columns.isEmpty()) { - if (definedCollections != null) - throw new InvalidRequestException("Collection types are not supported with COMPACT STORAGE"); - stmt.comparator = CFDefinition.definitionType; + if (columnAliases.isEmpty()) + throw new InvalidRequestException(String.format("COMPACT STORAGE with non-composite PRIMARY KEY require one column not part of the PRIMARY KEY (got: %s)", StringUtils.join(stmt.columns.keySet(), ", "))); + + // The only value we'll insert will be the empty one, so the default validator don't matter + stmt.defaultValidator = CFDefinition.definitionType; + // We need to distinguish between + // * I'm upgrading from thrift so the valueAlias is null + // * I've define my table with only a PK (and the column value will be empty) + // So, we use an empty valueAlias (rather than null) for the second case + stmt.valueAlias = ByteBufferUtil.EMPTY_BYTE_BUFFER; } else { - List<AbstractType<?>> types = new ArrayList<AbstractType<?>>(definedCollections == null ? 1 : 2); - types.add(CFDefinition.definitionType); - if (definedCollections != null) - types.add(ColumnToCollectionType.getInstance(definedCollections)); - stmt.comparator = CompositeType.getInstance(types); + Map.Entry<ColumnIdentifier, AbstractType> lastEntry = stmt.columns.entrySet().iterator().next(); + stmt.defaultValidator = lastEntry.getValue(); + stmt.valueAlias = lastEntry.getKey().key; + stmt.columns.remove(lastEntry.getKey()); } } - - if (stmt.columns.isEmpty()) - throw new InvalidRequestException("No definition found that is not part of the PRIMARY KEY"); - - if (useCompactStorage && stmt.columns.size() == 1) - { - Map.Entry<ColumnIdentifier, AbstractType> lastEntry = stmt.columns.entrySet().iterator().next(); - stmt.defaultValidator = lastEntry.getValue(); - stmt.valueAlias = lastEntry.getKey().key; - stmt.columns.remove(lastEntry.getKey()); - } else { if (useCompactStorage && !columnAliases.isEmpty()) - throw new InvalidRequestException(String.format("COMPACT STORAGE with composite PRIMARY KEY allows only one column not part of the PRIMARY KEY (got: %s)", StringUtils.join(stmt.columns.keySet(), ", "))); + throw new InvalidRequestException(String.format("COMPACT STORAGE with composite PRIMARY KEY allows no more than one column not part of the PRIMARY KEY (got: %s)", StringUtils.join(stmt.columns.keySet(), ", "))); // There is no way to insert/access a column that is not defined for non-compact storage, so // the actual validator don't matter much (except that we want to recognize counter CF as limitation apply to them). - stmt.defaultValidator = (stmt.columns.values().iterator().next() instanceof CounterColumnType) ? CounterColumnType.instance : CFDefinition.definitionType; + stmt.defaultValidator = !stmt.columns.isEmpty() && (stmt.columns.values().iterator().next() instanceof CounterColumnType) + ? CounterColumnType.instance + : CFDefinition.definitionType; } return new ParsedStatement.Prepared(stmt); http://git-wip-us.apache.org/repos/asf/cassandra/blob/79517461/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java index 97ff931..d370c28 100644 --- a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java @@ -270,10 +270,11 @@ public class SelectStatement implements CQLStatement { if (isColumnRange()) { - // For sparse, we used to ask for 'defined columns' * 'asked limit' to account for the grouping of columns. - // Since that doesn't work for maps/sets/lists, we use the compositesToGroup option of SliceQueryFilter. - // But we must preserve backward compatibility too. - int multiplier = cfDef.isCompact ? 1 : cfDef.metadata.size(); + // For sparse, we used to ask for 'defined columns' * 'asked limit' (where defined columns includes the row marker) + // to account for the grouping of columns. + // Since that doesn't work for maps/sets/lists, we now use the compositesToGroup option of SliceQueryFilter. + // But we must preserve backward compatibility too (for mixed version cluster that is). + int multiplier = cfDef.isCompact ? 1 : (cfDef.metadata.size() + 1); int toGroup = cfDef.isCompact ? -1 : cfDef.columns.size(); ColumnSlice slice = new ColumnSlice(getRequestedBound(isReversed ? Bound.END : Bound.START, variables), getRequestedBound(isReversed ? Bound.START : Bound.END, variables)); @@ -395,6 +396,7 @@ public class SelectStatement implements CQLStatement assert r != null && r.isEquality(); if (r.eqValues.size() > 1) { + assert cfDef.isCompact; // We have a IN. We only support this for the last column, so just create all columns and return. SortedSet<ByteBuffer> columns = new TreeSet<ByteBuffer>(cfDef.cfm.comparator); Iterator<Term> iter = r.eqValues.iterator(); @@ -423,19 +425,31 @@ public class SelectStatement implements CQLStatement // non-know set of columns, so we shouldn't get there assert !cfDef.hasCollections; - // Adds all columns (even if the user selected a few columns, we - // need to query all columns to know if the row exists or not). - // Note that when we allow IS NOT NULL in queries and if all - // selected name are request 'not null', we will allow to only - // query those. SortedSet<ByteBuffer> columns = new TreeSet<ByteBuffer>(cfDef.cfm.comparator); - Iterator<ColumnIdentifier> iter = cfDef.metadata.keySet().iterator(); - while (iter.hasNext()) + + // We need to query the selected column as well as the marker + // column (for the case where the row exists but has no columns outside the PK) + // One exception is "static CF" (non-composite non-compact CF) that + // don't have marker and for which we must query all columns instead + if (cfDef.isComposite) + { + // marker + columns.add(builder.copy().add(ByteBufferUtil.EMPTY_BYTE_BUFFER).build()); + + // selected columns + for (Pair<CFDefinition.Name, Selector> p : getExpandedSelection()) + columns.add(builder.copy().add(p.right.id().key).build()); + } + else { - ColumnIdentifier name = iter.next(); - ColumnNameBuilder b = iter.hasNext() ? builder.copy() : builder; - ByteBuffer cname = b.add(name.key).build(); - columns.add(cname); + Iterator<ColumnIdentifier> iter = cfDef.metadata.keySet().iterator(); + while (iter.hasNext()) + { + ColumnIdentifier name = iter.next(); + ColumnNameBuilder b = iter.hasNext() ? builder.copy() : builder; + ByteBuffer cname = b.add(name.key).build(); + columns.add(cname); + } } return columns; } @@ -948,10 +962,11 @@ public class SelectStatement implements CQLStatement if (!cfDef.isComposite && (!restriction.isInclusive(Bound.START) || !restriction.isInclusive(Bound.END))) stmt.sliceRestriction = restriction; } - // We only support IN for the last name so far - else if (restriction.eqValues.size() > 1 && i != stmt.columnRestrictions.length - 1) + // We only support IN for the last name and for compact storage so far + // TODO: #3885 allows us to extend to non compact as well, but that remains to be done + else if (cfDef.isCompact && restriction.eqValues.size() > 1 && i != stmt.columnRestrictions.length - 1) { - throw new InvalidRequestException(String.format("PRIMARY KEY part %s cannot be restricted by IN relation (only the first and last parts can)", cname)); + throw new InvalidRequestException(String.format("PRIMARY KEY part %s cannot be restricted by IN relation", cname)); } previous = cname; http://git-wip-us.apache.org/repos/asf/cassandra/blob/79517461/src/java/org/apache/cassandra/cql3/statements/UpdateStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/UpdateStatement.java b/src/java/org/apache/cassandra/cql3/statements/UpdateStatement.java index f84875a..908c321 100644 --- a/src/java/org/apache/cassandra/cql3/statements/UpdateStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/UpdateStatement.java @@ -25,12 +25,14 @@ import com.google.common.collect.ArrayListMultimap; import org.apache.cassandra.cql3.*; import org.apache.cassandra.config.CFMetaData; +import org.apache.cassandra.cql3.operations.ColumnOperation; import org.apache.cassandra.cql3.operations.Operation; import org.apache.cassandra.db.*; import org.apache.cassandra.db.marshal.*; import org.apache.cassandra.service.ClientState; import org.apache.cassandra.thrift.InvalidRequestException; import org.apache.cassandra.thrift.UnavailableException; +import org.apache.cassandra.utils.ByteBufferUtil; import org.apache.cassandra.utils.Pair; import static org.apache.cassandra.cql.QueryProcessor.validateKey; @@ -182,16 +184,41 @@ public class UpdateStatement extends ModificationStatement RowMutation rm = new RowMutation(cfDef.cfm.ksName, key); ColumnFamily cf = rm.addOrGet(cfDef.cfm.cfName); + // Inserting the CQL row marker (see #4361) + // We always need to insert a marker, because of the following situation: + // CREATE TABLE t ( k int PRIMARY KEY, c text ); + // INSERT INTO t(k, c) VALUES (1, 1) + // DELETE c FROM t WHERE k = 1; + // SELECT * FROM t; + // The last query should return one row (but with c == null). Adding + // the marker with the insert make sure the semantic is correct (while making sure a + // 'DELETE FROM t WHERE k = 1' does remove the row entirely) + if (cfDef.isComposite && !cfDef.isCompact) + { + ByteBuffer name = builder.copy().add(ByteBufferUtil.EMPTY_BYTE_BUFFER).build(); + cf.addColumn(params.makeColumn(name, ByteBufferUtil.EMPTY_BYTE_BUFFER)); + } + if (cfDef.isCompact) { if (builder.componentCount() == 0) throw new InvalidRequestException(String.format("Missing PRIMARY KEY part %s", cfDef.columns.values().iterator().next())); - List<Operation> operation = processedColumns.get(cfDef.value); - if (operation.isEmpty()) - throw new InvalidRequestException(String.format("Missing mandatory column %s", cfDef.value)); - assert operation.size() == 1; - hasCounterColumn = addToMutation(cf, builder, cfDef.value, operation.get(0), params, null); + Operation operation; + if (cfDef.value == null) + { + // No value was defined, we set to the empty value + operation = ColumnOperation.SetToEmpty(); + } + else + { + List<Operation> operations = processedColumns.get(cfDef.value); + if (operations.isEmpty()) + throw new InvalidRequestException(String.format("Missing mandatory column %s", cfDef.value)); + assert operations.size() == 1; + operation = operations.get(0); + } + hasCounterColumn = addToMutation(cf, builder, cfDef.value, operation, params, null); } else { @@ -212,15 +239,15 @@ public class UpdateStatement extends ModificationStatement UpdateParameters params, List<Pair<ByteBuffer, IColumn>> list) throws InvalidRequestException { - Operation.Type type = valueOperation.getType(); if (type == Operation.Type.COLUMN || type == Operation.Type.COUNTER) { - if (valueDef.type.isCollection()) + if (valueDef != null && valueDef.type.isCollection()) throw new InvalidRequestException("Can't apply operation on column with " + valueDef.type + " type."); - valueOperation.execute(cf, builder.copy(), valueDef.type, params); + AbstractType<?> validator = valueDef == null ? null : valueDef.type; + valueOperation.execute(cf, builder.copy(), validator, params); } else {
