diff --git a/CHANGES.txt b/CHANGES.txt index bc6db0dd53..f587a55e6d 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -8,6 +8,8 @@ 2.1.0-rc1 * Parallel streaming for sstableloader (CASSANDRA-3668) + * Fix bugs in supercolumns handling (CASSANDRA-7138) + * Fix ClassClassException on composite dense tables (CASSANDRA-7112) Merged from 2.0: * Make batchlog replica selection rack-aware (CASSANDRA-6551) * Suggest CTRL-C or semicolon after three blank lines in cqlsh (CASSANDRA-7142) diff --git a/interface/cassandra.thrift b/interface/cassandra.thrift index 303cfe4158..0af7e645cd 100644 --- a/interface/cassandra.thrift +++ b/interface/cassandra.thrift @@ -467,7 +467,6 @@ struct CfDef { 33: optional double bloom_filter_fp_chance, 34: optional string caching="keys_only", 37: optional double dclocal_read_repair_chance = 0.0, - 38: optional bool populate_io_cache_on_flush, 39: optional i32 memtable_flush_period_in_ms, 40: optional i32 default_time_to_live, 42: optional string speculative_retry="NONE", @@ -501,6 +500,8 @@ struct CfDef { /** @deprecated */ 31: optional i32 row_cache_keys_to_save, /** @deprecated */ + 38: optional bool populate_io_cache_on_flush, + /** @deprecated */ 41: optional i32 index_interval, } diff --git a/src/java/org/apache/cassandra/config/CFMetaData.java b/src/java/org/apache/cassandra/config/CFMetaData.java index 695760e7b4..e15535bbf7 100644 --- a/src/java/org/apache/cassandra/config/CFMetaData.java +++ b/src/java/org/apache/cassandra/config/CFMetaData.java @@ -58,7 +58,6 @@ import org.apache.cassandra.cql3.statements.CreateTableStatement; import org.apache.cassandra.db.AbstractCell; import org.apache.cassandra.db.AtomDeserializer; import org.apache.cassandra.db.CFRowAdder; -import org.apache.cassandra.db.Cell; import org.apache.cassandra.db.ColumnFamily; import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.db.ColumnFamilyType; @@ -69,7 +68,6 @@ import org.apache.cassandra.db.Mutation; import org.apache.cassandra.db.OnDiskAtom; import org.apache.cassandra.db.RangeTombstone; import org.apache.cassandra.db.Row; -import org.apache.cassandra.db.SuperColumns; import org.apache.cassandra.db.SystemKeyspace; import org.apache.cassandra.db.compaction.AbstractCompactionStrategy; import org.apache.cassandra.db.compaction.LeveledCompactionStrategy; @@ -97,6 +95,7 @@ import org.apache.cassandra.io.compress.CompressionParameters; import org.apache.cassandra.io.compress.LZ4Compressor; import org.apache.cassandra.io.sstable.Descriptor; import org.apache.cassandra.serializers.MarshalException; +import org.apache.cassandra.thrift.CfDef; import org.apache.cassandra.thrift.CqlResult; import org.apache.cassandra.thrift.CqlRow; import org.apache.cassandra.tracing.Tracing; @@ -988,45 +987,18 @@ public final class CFMetaData } } - public static CFMetaData fromThrift(org.apache.cassandra.thrift.CfDef cf_def) throws InvalidRequestException, ConfigurationException + public static CFMetaData fromThrift(CfDef cf_def) throws InvalidRequestException, ConfigurationException { - CFMetaData cfm = internalFromThrift(cf_def); - - if (cf_def.isSetKey_alias() && !(cfm.keyValidator instanceof CompositeType)) - cfm.addOrReplaceColumnDefinition(ColumnDefinition.partitionKeyDef(cfm, cf_def.key_alias, cfm.keyValidator, null)); - - return cfm.rebuild(); + return internalFromThrift(cf_def, Collections.emptyList()); } - public static CFMetaData fromThriftForUpdate(org.apache.cassandra.thrift.CfDef cf_def, CFMetaData toUpdate) throws InvalidRequestException, ConfigurationException + public static CFMetaData fromThriftForUpdate(CfDef cf_def, CFMetaData toUpdate) throws InvalidRequestException, ConfigurationException { - CFMetaData cfm = internalFromThrift(cf_def); - - // Thrift update can't have CQL metadata, and so we'll copy the ones of the updated metadata (to make - // sure we don't override anything existing -- see #6831). One exception (for historical reasons) is - // the partition key column name however, which can be provided through thrift. If it is, make sure - // we use the one of the update. - boolean hasKeyAlias = cf_def.isSetKey_alias() && !(cfm.keyValidator instanceof CompositeType); - if (hasKeyAlias) - cfm.addOrReplaceColumnDefinition(ColumnDefinition.partitionKeyDef(cfm, cf_def.key_alias, cfm.keyValidator, null)); - - for (ColumnDefinition def : toUpdate.allColumns()) - { - // isPartOfCellName basically means 'is not just a CQL metadata' - if (def.isPartOfCellName()) - continue; - - if (def.kind == ColumnDefinition.Kind.PARTITION_KEY && hasKeyAlias) - continue; - - cfm.addOrReplaceColumnDefinition(def); - } - - return cfm.rebuild(); + return internalFromThrift(cf_def, toUpdate.allColumns()); } - // Do most of the work, but don't handle CQL metadata (i.e. skip key_alias and don't rebuild()) - private static CFMetaData internalFromThrift(org.apache.cassandra.thrift.CfDef cf_def) throws InvalidRequestException, ConfigurationException + // Convert a thrift CfDef, given a list of ColumnDefinitions to copy over to the created CFMetadata before the CQL metadata are rebuild + private static CFMetaData internalFromThrift(CfDef cf_def, Collection previousCQLMetadata) throws InvalidRequestException, ConfigurationException { ColumnFamilyType cfType = ColumnFamilyType.create(cf_def.column_type); if (cfType == null) @@ -1041,25 +1013,52 @@ public final class CFMetaData ? null : cf_def.subcomparator_type == null ? BytesType.instance : TypeParser.parse(cf_def.subcomparator_type); - // Dense for thrit is simplified as all column metadata are REGULAR - boolean isDense = (cf_def.column_metadata == null || cf_def.column_metadata.isEmpty()) && !isCQL3OnlyPKComparator(rawComparator); - CellNameType comparator = CellNames.fromAbstractType(makeRawAbstractType(rawComparator, subComparator), isDense); + AbstractType fullRawComparator = makeRawAbstractType(rawComparator, subComparator); + + AbstractType keyValidator = cf_def.isSetKey_validation_class() ? TypeParser.parse(cf_def.key_validation_class) : null; + + // Convert the REGULAR definitions from the input CfDef + List defs = ColumnDefinition.fromThrift(cf_def.keyspace, cf_def.name, rawComparator, subComparator, cf_def.column_metadata); + + // Add the keyAlias if there is one, since that's on CQL metadata that thrift can actually change (for + // historical reasons) + boolean hasKeyAlias = cf_def.isSetKey_alias() && keyValidator != null && !(keyValidator instanceof CompositeType); + if (hasKeyAlias) + defs.add(ColumnDefinition.partitionKeyDef(cf_def.keyspace, cf_def.name, cf_def.key_alias, keyValidator, null)); + + // Now add any CQL metadata that we want to copy, skipping the keyAlias if there was one + for (ColumnDefinition def : previousCQLMetadata) + { + // isPartOfCellName basically means 'is not just a CQL metadata' + if (def.isPartOfCellName()) + continue; + + if (def.kind == ColumnDefinition.Kind.PARTITION_KEY && hasKeyAlias) + continue; + + defs.add(def); + } + + CellNameType comparator = CellNames.fromAbstractType(fullRawComparator, isDense(fullRawComparator, defs)); UUID cfId = Schema.instance.getId(cf_def.keyspace, cf_def.name); if (cfId == null) cfId = UUIDGen.getTimeUUID(); - CFMetaData newCFMD = new CFMetaData(cf_def.keyspace, - cf_def.name, - cfType, - comparator, - cfId); + CFMetaData newCFMD = new CFMetaData(cf_def.keyspace, cf_def.name, cfType, comparator, cfId); - if (cf_def.isSetGc_grace_seconds()) { newCFMD.gcGraceSeconds(cf_def.gc_grace_seconds); } - if (cf_def.isSetMin_compaction_threshold()) { newCFMD.minCompactionThreshold(cf_def.min_compaction_threshold); } - if (cf_def.isSetMax_compaction_threshold()) { newCFMD.maxCompactionThreshold(cf_def.max_compaction_threshold); } + newCFMD.addAllColumnDefinitions(defs); + + if (keyValidator != null) + newCFMD.keyValidator(keyValidator); + if (cf_def.isSetGc_grace_seconds()) + newCFMD.gcGraceSeconds(cf_def.gc_grace_seconds); + if (cf_def.isSetMin_compaction_threshold()) + newCFMD.minCompactionThreshold(cf_def.min_compaction_threshold); + if (cf_def.isSetMax_compaction_threshold()) + newCFMD.maxCompactionThreshold(cf_def.max_compaction_threshold); if (cf_def.isSetCompaction_strategy()) - newCFMD.compactionStrategyClass = createCompactionStrategy(cf_def.compaction_strategy); + newCFMD.compactionStrategyClass(createCompactionStrategy(cf_def.compaction_strategy)); if (cf_def.isSetCompaction_strategy_options()) newCFMD.compactionStrategyOptions(new HashMap<>(cf_def.compaction_strategy_options)); if (cf_def.isSetBloom_filter_fp_chance()) @@ -1080,18 +1079,13 @@ public final class CFMetaData newCFMD.maxIndexInterval(cf_def.max_index_interval); if (cf_def.isSetSpeculative_retry()) newCFMD.speculativeRetry(SpeculativeRetry.fromString(cf_def.speculative_retry)); - if (cf_def.isSetPopulate_io_cache_on_flush()) if (cf_def.isSetTriggers()) newCFMD.triggers(TriggerDefinition.fromThrift(cf_def.triggers)); - CompressionParameters cp = CompressionParameters.create(cf_def.compression_options); - - if (cf_def.isSetKey_validation_class()) { newCFMD.keyValidator(TypeParser.parse(cf_def.key_validation_class)); } - - return newCFMD.addAllColumnDefinitions(ColumnDefinition.fromThrift(newCFMD, cf_def.column_metadata)) - .comment(cf_def.comment) + return newCFMD.comment(cf_def.comment) .defaultValidator(TypeParser.parse(cf_def.default_validation_class)) - .compressionParameters(cp); + .compressionParameters(CompressionParameters.create(cf_def.compression_options)) + .rebuild(); } catch (SyntaxException | MarshalException e) { @@ -1777,7 +1771,6 @@ public final class CFMetaData cfm.minIndexInterval(result.getInt("min_index_interval")); else if (result.has("index_interval")) cfm.minIndexInterval(result.getInt("index_interval")); - if (result.has("max_index_interval")) cfm.maxIndexInterval(result.getInt("max_index_interval")); @@ -1792,7 +1785,6 @@ public final class CFMetaData cfm.addColumnMetadataFromAliases(aliasesFromStrings(fromJsonList(result.getString("key_aliases"))), cfm.keyValidator, ColumnDefinition.Kind.PARTITION_KEY); if (result.has("column_aliases")) cfm.addColumnMetadataFromAliases(aliasesFromStrings(fromJsonList(result.getString("column_aliases"))), cfm.comparator.asAbstractType(), ColumnDefinition.Kind.CLUSTERING_COLUMN); - if (result.has("value_alias")) cfm.addColumnMetadataFromAliases(Collections.singletonList(result.getBytes("value_alias")), cfm.defaultValidator, ColumnDefinition.Kind.COMPACT_VALUE); @@ -2229,7 +2221,7 @@ public final class CFMetaData .append("comparator", comparator) .append("comment", comment) .append("readRepairChance", readRepairChance) - .append("dclocalReadRepairChance", dcLocalReadRepairChance) + .append("dcLocalReadRepairChance", dcLocalReadRepairChance) .append("gcGraceSeconds", gcGraceSeconds) .append("defaultValidator", defaultValidator) .append("keyValidator", keyValidator) diff --git a/src/java/org/apache/cassandra/config/ColumnDefinition.java b/src/java/org/apache/cassandra/config/ColumnDefinition.java index caffa63da4..14c1ee398d 100644 --- a/src/java/org/apache/cassandra/config/ColumnDefinition.java +++ b/src/java/org/apache/cassandra/config/ColumnDefinition.java @@ -98,6 +98,11 @@ public class ColumnDefinition extends ColumnSpecification return new ColumnDefinition(cfm, name, validator, componentIndex, Kind.PARTITION_KEY); } + public static ColumnDefinition partitionKeyDef(String ksName, String cfName, ByteBuffer name, AbstractType validator, Integer componentIndex) + { + return new ColumnDefinition(ksName, cfName, new ColumnIdentifier(name, UTF8Type.instance), validator, null, null, null, componentIndex, Kind.PARTITION_KEY); + } + public static ColumnDefinition clusteringKeyDef(CFMetaData cfm, ByteBuffer name, AbstractType validator, Integer componentIndex) { return new ColumnDefinition(cfm, name, validator, componentIndex, Kind.CLUSTERING_COLUMN); @@ -265,11 +270,11 @@ public class ColumnDefinition extends ColumnSpecification return cd; } - public static ColumnDefinition fromThrift(CFMetaData cfm, ColumnDef thriftColumnDef) throws SyntaxException, ConfigurationException + public static ColumnDefinition fromThrift(String ksName, String cfName, AbstractType thriftComparator, AbstractType thriftSubcomparator, ColumnDef thriftColumnDef) throws SyntaxException, ConfigurationException { // For super columns, the componentIndex is 1 because the ColumnDefinition applies to the column component. - Integer componentIndex = cfm.isSuper() ? 1 : null; - AbstractType comparator = cfm.getComponentComparator(componentIndex, Kind.REGULAR); + Integer componentIndex = thriftSubcomparator != null ? 1 : null; + AbstractType comparator = thriftSubcomparator == null ? thriftComparator : thriftSubcomparator; try { comparator.validate(thriftColumnDef.name); @@ -279,26 +284,25 @@ public class ColumnDefinition extends ColumnSpecification throw new ConfigurationException(String.format("Column name %s is not valid for comparator %s", ByteBufferUtil.bytesToHex(thriftColumnDef.name), comparator)); } - ColumnDefinition cd = new ColumnDefinition(cfm, - ByteBufferUtil.clone(thriftColumnDef.name), - TypeParser.parse(thriftColumnDef.validation_class), - componentIndex, - Kind.REGULAR); - - cd.setIndex(thriftColumnDef.index_name, - thriftColumnDef.index_type == null ? null : IndexType.valueOf(thriftColumnDef.index_type.name()), - thriftColumnDef.index_options); - return cd; + return new ColumnDefinition(ksName, + cfName, + new ColumnIdentifier(ByteBufferUtil.clone(thriftColumnDef.name), comparator), + TypeParser.parse(thriftColumnDef.validation_class), + thriftColumnDef.index_type == null ? null : IndexType.valueOf(thriftColumnDef.index_type.name()), + thriftColumnDef.index_options, + thriftColumnDef.index_name, + componentIndex, + Kind.REGULAR); } - public static List fromThrift(CFMetaData cfm, List thriftDefs) throws SyntaxException, ConfigurationException + public static List fromThrift(String ksName, String cfName, AbstractType thriftComparator, AbstractType thriftSubcomparator, List thriftDefs) throws SyntaxException, ConfigurationException { if (thriftDefs == null) return Collections.emptyList(); List defs = new ArrayList<>(thriftDefs.size()); for (ColumnDef thriftColumnDef : thriftDefs) - defs.add(fromThrift(cfm, thriftColumnDef)); + defs.add(fromThrift(ksName, cfName, thriftComparator, thriftSubcomparator, thriftColumnDef)); return defs; } diff --git a/src/java/org/apache/cassandra/db/SuperColumns.java b/src/java/org/apache/cassandra/db/SuperColumns.java index bab46ebe3d..2006cbd227 100644 --- a/src/java/org/apache/cassandra/db/SuperColumns.java +++ b/src/java/org/apache/cassandra/db/SuperColumns.java @@ -184,7 +184,10 @@ public class SuperColumns int i = 0; for (CellName name : filter.columns) { - slices[i++] = name.slice(); + // Note that, because the filter in argument is the one from thrift, 'name' are SimpleDenseCellName. + // So calling name.slice() would be incorrect, as simple cell names don't handle the EOC properly. + // This is why we call toByteBuffer() and rebuild a Composite of the right type before call slice(). + slices[i++] = type.make(name.toByteBuffer()).slice(); } return new SliceQueryFilter(slices, false, slices.length, 1); } diff --git a/src/java/org/apache/cassandra/db/composites/CompoundDenseCellNameType.java b/src/java/org/apache/cassandra/db/composites/CompoundDenseCellNameType.java index 0fd2f4b3ce..e3c2831a03 100644 --- a/src/java/org/apache/cassandra/db/composites/CompoundDenseCellNameType.java +++ b/src/java/org/apache/cassandra/db/composites/CompoundDenseCellNameType.java @@ -67,10 +67,10 @@ public class CompoundDenseCellNameType extends AbstractCompoundCellNameType protected Composite makeWith(ByteBuffer[] components, int size, Composite.EOC eoc, boolean isStatic) { assert !isStatic; - if (size < fullSize || eoc != Composite.EOC.NONE) - return new CompoundComposite(components, size, false).withEOC(eoc); - - return new CompoundDenseCellName(components, size); + // A composite dense table cell name don't have to have all the component set to qualify as a + // proper CellName (for backward compatibility reasons mostly), so always return a cellName + Composite c = new CompoundDenseCellName(components, size); + return eoc != Composite.EOC.NONE ? c.withEOC(eoc) : c; } protected Composite copyAndMakeWith(ByteBuffer[] components, int size, Composite.EOC eoc, boolean isStatic) diff --git a/src/java/org/apache/cassandra/thrift/CassandraServer.java b/src/java/org/apache/cassandra/thrift/CassandraServer.java index 1d138184cd..4f54242a1c 100644 --- a/src/java/org/apache/cassandra/thrift/CassandraServer.java +++ b/src/java/org/apache/cassandra/thrift/CassandraServer.java @@ -376,12 +376,18 @@ public class CassandraServer implements Cassandra.Iface private SliceQueryFilter toInternalFilter(CFMetaData metadata, ColumnParent parent, SliceRange range) { + if (metadata.isSuper()) + { + CellNameType columnType = new SimpleDenseCellNameType(metadata.comparator.subtype(parent.isSetSuper_column() ? 1 : 0)); + Composite start = columnType.fromByteBuffer(range.start); + Composite finish = columnType.fromByteBuffer(range.finish); + SliceQueryFilter filter = new SliceQueryFilter(start, finish, range.reversed, range.count); + return SuperColumns.fromSCSliceFilter(metadata.comparator, parent.bufferForSuper_column(), filter); + } + Composite start = metadata.comparator.fromByteBuffer(range.start); Composite finish = metadata.comparator.fromByteBuffer(range.finish); - SliceQueryFilter filter = new SliceQueryFilter(start, finish, range.reversed, range.count); - if (metadata.isSuper()) - filter = SuperColumns.fromSCSliceFilter(metadata.comparator, parent.bufferForSuper_column(), filter); - return filter; + return new SliceQueryFilter(start, finish, range.reversed, range.count); } private IDiskAtomFilter toInternalFilter(CFMetaData metadata, ColumnParent parent, SlicePredicate predicate) diff --git a/test/unit/org/apache/cassandra/config/ColumnDefinitionTest.java b/test/unit/org/apache/cassandra/config/ColumnDefinitionTest.java index 8ae2d5f61e..890c46c746 100644 --- a/test/unit/org/apache/cassandra/config/ColumnDefinitionTest.java +++ b/test/unit/org/apache/cassandra/config/ColumnDefinitionTest.java @@ -45,7 +45,7 @@ public class ColumnDefinitionTest protected void testSerializeDeserialize(CFMetaData cfm, ColumnDefinition cd) throws Exception { - ColumnDefinition newCd = ColumnDefinition.fromThrift(cfm, cd.toThrift()); + ColumnDefinition newCd = ColumnDefinition.fromThrift(cfm.ksName, cfm.cfName, cfm.comparator.asAbstractType(), null, cd.toThrift()); Assert.assertNotSame(cd, newCd); Assert.assertEquals(cd.hashCode(), newCd.hashCode()); Assert.assertEquals(cd, newCd);