From 91980589866f3425f1f3c64a959be1779be73618 Mon Sep 17 00:00:00 2001 From: Dmitry Konstantinov Date: Mon, 25 May 2026 19:35:46 +0100 Subject: [PATCH] Avoid type lookup in SerializationHeader#getType if schema and SSTable are aligned patch by Dmitry Konstantinov; reviewed by Francisco Guerrero for CASSANDRA-21402 --- CHANGES.txt | 1 + .../cassandra/db/SerializationHeader.java | 14 +- .../cassandra/db/SerializationHeaderTest.java | 170 +++++++++++++++++- 3 files changed, 180 insertions(+), 5 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 8d362199a8..2b666949d5 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 6.0-alpha2 + * Avoid type lookup in SerializationHeader#getType if schema and SSTable are aligned (CASSANDRA-21402) * Replace LongAdder in metric-like logic with ThreadLocalCounter (CASSANDRA-21400) * BTreeRow.hasLiveData: avoid Cell iteration if there are no cell tombstones (CASSANDRA-21363) * Reduce memory allocations in row merge logic (CASSANDRA-21359) diff --git a/src/java/org/apache/cassandra/db/SerializationHeader.java b/src/java/org/apache/cassandra/db/SerializationHeader.java index 17b2888a89..dd4ea5a495 100644 --- a/src/java/org/apache/cassandra/db/SerializationHeader.java +++ b/src/java/org/apache/cassandra/db/SerializationHeader.java @@ -349,6 +349,7 @@ public class SerializationHeader public SerializationHeader toHeader(TableMetadata metadata) throws UnknownColumnException { Map> typeMap = new HashMap<>(staticColumns.size() + regularColumns.size()); + boolean hasTypeMismatch = false; RegularAndStaticColumns.Builder builder = RegularAndStaticColumns.builder(); for (Map> map : ImmutableList.of(staticColumns, regularColumns)) @@ -357,9 +358,10 @@ public class SerializationHeader for (Map.Entry> e : map.entrySet()) { ByteBuffer name = e.getKey(); - AbstractType other = typeMap.put(name, e.getValue()); - if (other != null && !other.equals(e.getValue())) - throw new IllegalStateException("Column " + name + " occurs as both regular and static with types " + other + "and " + e.getValue()); + AbstractType diskType = e.getValue(); + AbstractType other = typeMap.put(name, diskType); + if (other != null && !other.equals(diskType)) + throw new IllegalStateException("Column " + name + " occurs as both regular and static with types " + other + "and " + diskType); ColumnMetadata column = metadata.getColumn(name); if (column == null || column.isStatic() != isStatic) @@ -376,11 +378,15 @@ public class SerializationHeader if (column == null) throw new UnknownColumnException("Unknown column " + UTF8Type.instance.getString(name) + " during deserialization"); } + if (!diskType.equals(column.type)) + hasTypeMismatch = true; builder.add(column); } } - return new SerializationHeader(true, keyType, clusteringTypes, builder.build(), stats, typeMap); + // if all on-disk types match the current schema then we can use + // fast column.type path on every cell read instead of doing a map lookup. + return new SerializationHeader(true, keyType, clusteringTypes, builder.build(), stats, hasTypeMismatch ? typeMap : null); } @Override diff --git a/test/unit/org/apache/cassandra/db/SerializationHeaderTest.java b/test/unit/org/apache/cassandra/db/SerializationHeaderTest.java index 33f1a63238..0993606739 100644 --- a/test/unit/org/apache/cassandra/db/SerializationHeaderTest.java +++ b/test/unit/org/apache/cassandra/db/SerializationHeaderTest.java @@ -36,12 +36,15 @@ import org.apache.cassandra.cql3.ColumnIdentifier; import org.apache.cassandra.db.compaction.OperationType; import org.apache.cassandra.db.lifecycle.LifecycleTransaction; import org.apache.cassandra.db.marshal.Int32Type; +import org.apache.cassandra.db.marshal.ListType; +import org.apache.cassandra.db.marshal.LongType; import org.apache.cassandra.db.partitions.PartitionUpdate; import org.apache.cassandra.db.rows.BTreeRow; import org.apache.cassandra.db.rows.BufferCell; import org.apache.cassandra.db.rows.Cell; import org.apache.cassandra.db.rows.Row; import org.apache.cassandra.db.rows.UnfilteredRowIterator; +import org.apache.cassandra.exceptions.UnknownColumnException; import org.apache.cassandra.io.sstable.Descriptor; import org.apache.cassandra.io.sstable.ISSTableScanner; import org.apache.cassandra.io.sstable.SequenceBasedSSTableId; @@ -63,7 +66,172 @@ public class SerializationHeaderTest { DatabaseDescriptor.daemonInitialization(); } - + + /** + * Common case: schema unchanged since the SSTable was written. + */ + @Test + public void testTypeMapNullWhenNoSchemaChanges() throws UnknownColumnException + { + TableMetadata schema = TableMetadata.builder(KEYSPACE, "testTypeMapNull") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addRegularColumn("v", Int32Type.instance) + .build(); + + SerializationHeader.Component component = SerializationHeader.makeWithoutStats(schema).toComponent(); + SerializationHeader header = component.toHeader(schema); + + ColumnMetadata v = schema.getColumn(ColumnIdentifier.getInterned("v", false)); + // typeMap == null => getType() returns exactly column.type (same object reference) + Assert.assertSame(v.type, header.getType(v)); + } + + /** + * ALTER TABLE scenario: column was written as Int32Type, schema now has LongType. + * toHeader() must retain typeMap so getType() returns the on-disk Int32Type and + * not the current schema LongType, which would corrupt deserialization. + */ + @Test + public void testTypeMapNonNullWhenColumnTypeChanged() throws UnknownColumnException + { + ColumnIdentifier v = ColumnIdentifier.getInterned("v", false); + TableMetadata schemaAtWrite = TableMetadata.builder(KEYSPACE, "testTypeMapNonNull") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addRegularColumn("v", Int32Type.instance) + .build(); + TableMetadata schemaAfterAlter = TableMetadata.builder(KEYSPACE, "testTypeMapNonNull") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addRegularColumn("v", LongType.instance) // type is changed + .build(); + + SerializationHeader.Component component = SerializationHeader.makeWithoutStats(schemaAtWrite).toComponent(); + SerializationHeader header = component.toHeader(schemaAfterAlter); + + ColumnMetadata vNew = schemaAfterAlter.getColumn(v); + // getType() must return the on-disk Int32Type, not the current LongType + Assert.assertEquals(Int32Type.instance, header.getType(vNew)); + } + + /** + * Column was dropped at the same type it was written with — no actual type mismatch. + */ + @Test + public void testTypeMapNullForDroppedColumnWithSameType() throws UnknownColumnException + { + ColumnIdentifier v = ColumnIdentifier.getInterned("v", false); + TableMetadata schemaAtWrite = TableMetadata.builder(KEYSPACE, "testTypeMapDroppedSame") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addRegularColumn("v", Int32Type.instance) + .build(); + ColumnMetadata vColumn = schemaAtWrite.getColumn(v); + TableMetadata schemaAfterDrop = schemaAtWrite.unbuild().recordColumnDrop(vColumn, 0L).build(); + + SerializationHeader.Component component = SerializationHeader.makeWithoutStats(schemaAtWrite).toComponent(); + SerializationHeader header = component.toHeader(schemaAfterDrop); + + ColumnMetadata vDropped = schemaAfterDrop.getDroppedColumn(vColumn.name.bytes); + // disk type == dropped type => typeMap null => same reference + Assert.assertSame(vDropped.type, header.getType(vDropped)); + Assert.assertEquals(Int32Type.instance, header.getType(vDropped)); + } + + /** + * Column was altered from Int32Type to LongType and then dropped. + * An SSTable written before the ALTER stores Int32Type on disk, but the drop record + * has LongType. The type mismatch must be detected and typeMap retained so + * the Int32Type bytes can still be skipped correctly. + */ + @Test + public void testTypeMapNonNullForDroppedColumnWithChangedType() throws UnknownColumnException + { + ColumnIdentifier v = ColumnIdentifier.getInterned("v", false); + TableMetadata schemaAtWrite = TableMetadata.builder(KEYSPACE, "testTypeMapDroppedChanged") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addRegularColumn("v", Int32Type.instance) + .build(); + TableMetadata schemaWithAlteredType = TableMetadata.builder(KEYSPACE, "testTypeMapDroppedChanged") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addRegularColumn("v", LongType.instance) // type is changed + .build(); + ColumnMetadata vLong = schemaWithAlteredType.getColumn(v); + TableMetadata schemaAfterDrop = schemaWithAlteredType.unbuild() + .recordColumnDrop(vLong, 0L) // drop the value column + .build(); + + SerializationHeader.Component component = SerializationHeader.makeWithoutStats(schemaAtWrite).toComponent(); + SerializationHeader header = component.toHeader(schemaAfterDrop); + + ColumnMetadata vDropped = schemaAfterDrop.getDroppedColumn(vLong.name.bytes); + // disk type (Int32) != drop-record type (Long) => typeMap retained, returns Int32 + Assert.assertEquals(Int32Type.instance, header.getType(vDropped)); + Assert.assertNotSame(vDropped.type, header.getType(vDropped)); + } + + /** + * Static-column variant of {@link #testTypeMapNonNullWhenColumnTypeChanged}. + * The mismatch detection in toHeader() iterates staticColumns and regularColumns through + * the same loop; this covers the static branch. + */ + @Test + public void testTypeMapNonNullWhenStaticColumnTypeChanged() throws UnknownColumnException + { + ColumnIdentifier s = ColumnIdentifier.getInterned("s", false); + TableMetadata schemaAtWrite = TableMetadata.builder(KEYSPACE, "testTypeMapNonNullStatic") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addStaticColumn("s", Int32Type.instance) + .build(); + TableMetadata schemaAfterAlter = TableMetadata.builder(KEYSPACE, "testTypeMapNonNullStatic") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addStaticColumn("s", LongType.instance) // static column type is changed + .build(); + + SerializationHeader.Component component = SerializationHeader.makeWithoutStats(schemaAtWrite).toComponent(); + SerializationHeader header = component.toHeader(schemaAfterAlter); + + ColumnMetadata sNew = schemaAfterAlter.getColumn(s); + // getType() must return the on-disk Int32Type, not the current LongType + Assert.assertEquals(Int32Type.instance, header.getType(sNew)); + } + + /** + * Multi-cell vs frozen collections serialize differently and CollectionType.equals + * checks isMultiCell. A schema flipping a list from multi-cell to frozen (or vice versa) + * must be detected as a type mismatch so typeMap is retained. + */ + @Test + public void testTypeMapNonNullWhenCollectionMultiCellChanged() throws UnknownColumnException + { + ColumnIdentifier v = ColumnIdentifier.getInterned("v", false); + ListType multiCellList = ListType.getInstance(Int32Type.instance, true); + ListType frozenList = ListType.getInstance(Int32Type.instance, false); + + TableMetadata schemaAtWrite = TableMetadata.builder(KEYSPACE, "testTypeMapNonNullCollection") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addRegularColumn("v", multiCellList) + .build(); + TableMetadata schemaAfterFreeze = TableMetadata.builder(KEYSPACE, "testTypeMapNonNullCollection") + .addPartitionKeyColumn("k", Int32Type.instance) + .addClusteringColumn("c", Int32Type.instance) + .addRegularColumn("v", frozenList) // multi-cell -> frozen + .build(); + + SerializationHeader.Component component = SerializationHeader.makeWithoutStats(schemaAtWrite).toComponent(); + SerializationHeader header = component.toHeader(schemaAfterFreeze); + + ColumnMetadata vNew = schemaAfterFreeze.getColumn(v); + // getType() must return the on-disk multi-cell list, not the frozen schema type + Assert.assertEquals(multiCellList, header.getType(vNew)); + } + @Test public void testWrittenAsDifferentKind() throws Exception {