From b61c9233453a92589a3a06f13283437268dc1ee3 Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Sat, 5 Sep 2026 16:48:15 -0700 Subject: [PATCH] Canonicalize the default null value and intern per-column strings at metadata parse time A server retains one ColumnMetadataImpl + FieldSpec per (segment, column) for as long as the segment is loaded. For wide segments (1000+ columns, tens of thousands of segments per server) two per-column allocations made on the metadata.properties parse path add up to ~120 bytes per column that carry no information: - The segment creator writes `column..defaultNullValue` for every column, so `extractFieldSpec` handed the literal to the FieldSpec constructor, which boxed it (`Integer.valueOf("-2147483648")`) and kept the literal in the transient `_stringDefaultNullValue` - a fresh Integer, String and byte[] per column per segment even though the value is the type default. `ColumnMetadataImpl.extractFieldSpec` now parses the literal, compares it with `FieldSpec.getDefaultNullValue(fieldType, dataType, null)` under `DataType.equals` (the predicate `FieldSpec.equals`/`hashCode` use, byte[]-safe) and passes `null` when they match, so the spec holds the shared static `FieldSpec.DEFAULT_*` constant and retains no literal. A custom default is kept verbatim (after the STRING special-character recovery) and interned, so the segments of a table share it; a combination without a type default (a METRIC BOOLEAN with an explicit default) keeps parsing the literal as before. - Column names recur in every segment of a table, yet each segment parsed its own copy that was then retained six times over: the column metadata map key, FieldSpec._name, the Schema map key and dimension/metric list entry, and the loader's per-column maps. `SegmentMetadataImpl.addPhysicalColumns` now interns the parsed name, and `extractFieldSpec` interns COLUMN_NAME, PARENT_COLUMN, DATETIME_FORMAT, DATETIME_GRANULARITY and complex child names, so every String the metadata graph retains is one instance per distinct value per JVM. The JVM string table holds interned strings weakly, so they live exactly as long as a loaded segment references them and the table is bounded by the number of distinct column names; no code compares column names by identity. Compatibility: metadata.properties is read and written exactly as before (writers keep emitting defaultNullValue). The `/tables/{t}/segments/{s}/metadata` payload is unchanged because it bean-serializes `FieldSpec.getDefaultNullValue()` (a value that is equal by construction) - pinned by a test for every data type, including BYTES and UUID. FieldSpec.equals/hashCode ignore `_stringDefaultNullValue` and getDefaultNullValueString() derives from the value, so Schema equality against the table schema, default-column comparisons and reload decisions are unaffected. The only observable difference is that `FieldSpec.toJsonObject()` of a segment-derived BYTES column no longer emits a redundant `defaultNullValue: ""` (it compares the byte[] against the type default by identity, and the spec now holds the constant); no endpoint serializes a segment-derived Schema that way. No public signature changes; mixed-version safe (server-local, in-memory only). Co-Authored-By: Claude Fable 5.1 --- .../index/SegmentMetadataImplTest.java | 124 ++++++++++- .../index/metadata/ColumnMetadataImpl.java | 64 +++++- .../index/metadata/SegmentMetadataImpl.java | 7 +- .../metadata/ColumnMetadataImplTest.java | 208 +++++++++++++++++- 4 files changed, 391 insertions(+), 12 deletions(-) diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java index d4fa8d606188..08564230f148 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java @@ -19,16 +19,23 @@ package org.apache.pinot.segment.local.segment.index; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ObjectNode; import java.io.File; import java.io.FileInputStream; import java.io.IOException; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.commons.io.FileUtils; import org.apache.pinot.segment.local.segment.creator.SegmentTestUtils; import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl; import org.apache.pinot.segment.local.segment.index.converter.SegmentV1V2ToV3FormatConverter; +import org.apache.pinot.segment.local.segment.readers.GenericRowRecordReader; import org.apache.pinot.segment.spi.ColumnMetadata; import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig; import org.apache.pinot.segment.spi.creator.SegmentIndexCreationDriver; @@ -37,6 +44,18 @@ import org.apache.pinot.segment.spi.index.metadata.ColumnMetadataImpl; import org.apache.pinot.segment.spi.index.metadata.SegmentMetadataImpl; import org.apache.pinot.segment.spi.store.SegmentDirectoryPaths; +import org.apache.pinot.spi.config.table.FieldConfig; +import org.apache.pinot.spi.config.table.OpenStructIndexConfig; +import org.apache.pinot.spi.config.table.TableConfig; +import org.apache.pinot.spi.config.table.TableType; +import org.apache.pinot.spi.data.ComplexFieldSpec; +import org.apache.pinot.spi.data.DimensionFieldSpec; +import org.apache.pinot.spi.data.FieldSpec; +import org.apache.pinot.spi.data.OpenStructNaming; +import org.apache.pinot.spi.data.Schema; +import org.apache.pinot.spi.data.readers.GenericRow; +import org.apache.pinot.spi.utils.JsonUtils; +import org.apache.pinot.spi.utils.builder.TableConfigBuilder; import org.apache.pinot.util.TestUtils; import org.testng.Assert; import org.testng.annotations.AfterMethod; @@ -44,11 +63,13 @@ import org.testng.annotations.Test; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertSame; public class SegmentMetadataImplTest { private static final String AVRO_DATA = "data/test_data-mv.avro"; private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "SegmentMetadataImplTest"); + private File _avroFile; private File _segmentDirectory; @BeforeMethod @@ -56,10 +77,11 @@ public void setUp() throws Exception { final String filePath = TestUtils.getFileFromResourceUrl(SegmentMetadataImplTest.class.getClassLoader().getResource(AVRO_DATA)); + _avroFile = new File(filePath); // intentionally changed this to TimeUnit.Hours to make it non-default for testing final SegmentGeneratorConfig config = SegmentTestUtils - .getSegmentGenSpecWithSchemAndProjectedColumns(new File(filePath), INDEX_DIR, "daysSinceEpoch", TimeUnit.HOURS, + .getSegmentGenSpecWithSchemAndProjectedColumns(_avroFile, INDEX_DIR, "daysSinceEpoch", TimeUnit.HOURS, "testTable"); config.setSegmentNamePostfix("1"); config.setCustomProperties(Map.of("custom.k1", "v1", "custom.k2", "v2")); @@ -141,4 +163,104 @@ public void testIndexSizesOnlyFromIndexDir() Assert.assertTrue(((ColumnMetadataImpl) streamColumn).getIndexSizeMap().isEmpty(), entry.getKey()); } } + + /// A server holds one column-metadata graph per loaded segment, so the per-column strings and default null values + /// are shared: two loads of the same metadata alias one interned column name (as the map key, the FieldSpec name and + /// the Schema entry) and hold the static FieldSpec default constant rather than a box parsed from the literal the + /// segment creator wrote, while the specs stay equal to the schema the segment was built from. + @Test + public void testColumnStringsAndDefaultsSharedAcrossLoads() + throws Exception { + SegmentMetadataImpl first = new SegmentMetadataImpl(_segmentDirectory); + SegmentMetadataImpl second = new SegmentMetadataImpl(_segmentDirectory); + assertEquals(first.getColumnMetadataMap().keySet(), second.getColumnMetadataMap().keySet()); + assertSame(first.getColumnMetadataMap().firstKey(), second.getColumnMetadataMap().firstKey()); + assertSame(first.getSchema().getDimensionNames().get(0), second.getSchema().getDimensionNames().get(0)); + Iterator secondKeys = second.getColumnMetadataMap().keySet().iterator(); + for (String column : first.getColumnMetadataMap().keySet()) { + assertSame(column, secondKeys.next()); + FieldSpec fieldSpec = first.getColumnMetadataFor(column).getFieldSpec(); + assertSame(fieldSpec.getName(), column, column); + assertSame(fieldSpec.getName(), second.getColumnMetadataFor(column).getFieldSpec().getName(), column); + assertSame(first.getSchema().getFieldSpecFor(column).getName(), column, column); + assertSame(fieldSpec.getDefaultNullValue(), + FieldSpec.getDefaultNullValue(fieldSpec.getFieldType(), fieldSpec.getDataType(), null), column); + } + Schema inputSchema = SegmentTestUtils.extractSchemaFromAvroWithoutTime(_avroFile); + for (FieldSpec inputFieldSpec : inputSchema.getAllFieldSpecs()) { + assertEquals(first.getSchema().getFieldSpecFor(inputFieldSpec.getName()), inputFieldSpec); + } + } + + /// OPEN_STRUCT children carry an explicit column name and a parent column in `metadata.properties`; both come back + /// as the interned instances, so the child's parent name is the very String that keys the parent column. + @Test + public void testOpenStructChildStringsShared() + throws Exception { + String parent = "metrics"; + File segmentDir = buildOpenStructSegment(parent); + try { + SegmentMetadataImpl first = new SegmentMetadataImpl(segmentDir); + SegmentMetadataImpl second = new SegmentMetadataImpl(segmentDir); + String parentKey = first.getColumnMetadataMap().ceilingKey(parent); + assertEquals(parentKey, parent); + assertSame(parentKey, second.getColumnMetadataMap().ceilingKey(parent)); + String child = OpenStructNaming.materializedColumnName(parent, "cpu"); + ColumnMetadataImpl firstChild = (ColumnMetadataImpl) first.getColumnMetadataFor(child); + ColumnMetadataImpl secondChild = (ColumnMetadataImpl) second.getColumnMetadataFor(child); + assertEquals(firstChild.getParentColumn(), parent); + assertSame(firstChild.getParentColumn(), parentKey); + assertSame(firstChild.getParentColumn(), secondChild.getParentColumn()); + assertSame(firstChild.getFieldSpec().getName(), secondChild.getFieldSpec().getName()); + assertSame(firstChild.getFieldSpec().getDefaultNullValue(), FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_DOUBLE); + ComplexFieldSpec firstParent = (ComplexFieldSpec) first.getColumnMetadataFor(parent).getFieldSpec(); + ComplexFieldSpec secondParent = (ComplexFieldSpec) second.getColumnMetadataFor(parent).getFieldSpec(); + assertEquals(firstParent.getChildFieldSpecs().keySet(), Set.of("views", "cpu", "host")); + for (Map.Entry entry : firstParent.getChildFieldSpecs().entrySet()) { + FieldSpec childSpec = entry.getValue(); + assertSame(childSpec.getName(), entry.getKey()); + assertSame(childSpec.getName(), secondParent.getChildFieldSpec(entry.getKey()).getName()); + assertSame(childSpec.getDefaultNullValue(), + FieldSpec.getDefaultNullValue(childSpec.getFieldType(), childSpec.getDataType(), null)); + } + } finally { + FileUtils.deleteQuietly(segmentDir); + } + } + + private static File buildOpenStructSegment(String parent) + throws Exception { + Map children = new HashMap<>(); + children.put("views", new DimensionFieldSpec("views", FieldSpec.DataType.LONG, true)); + children.put("cpu", new DimensionFieldSpec("cpu", FieldSpec.DataType.DOUBLE, true)); + children.put("host", new DimensionFieldSpec("host", FieldSpec.DataType.STRING, true)); + Schema schema = new Schema.SchemaBuilder().setSchemaName("testOpenStruct") + .addField(new ComplexFieldSpec(parent, FieldSpec.DataType.OPEN_STRUCT, true, children)) + .addSingleValueDimension("dim", FieldSpec.DataType.STRING) + .build(); + OpenStructIndexConfig openStructConfig = + new OpenStructIndexConfig(false, null, 3, Set.of("views", "cpu", "host"), 0.5, List.of(), null); + ObjectNode indexes = JsonUtils.newObjectNode(); + indexes.set("open_struct", JsonUtils.objectToJsonNode(openStructConfig)); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testOpenStruct") + .setFieldConfigList(List.of(new FieldConfig.Builder(parent).withIndexes(indexes).build())).build(); + SegmentGeneratorConfig config = new SegmentGeneratorConfig(tableConfig, schema); + config.setOutDir(new File(INDEX_DIR, "openStruct").getAbsolutePath()); + config.setSegmentName("openStructSegment"); + List rows = new ArrayList<>(); + for (int i = 0; i < 10; i++) { + GenericRow row = new GenericRow(); + Map metrics = new HashMap<>(); + metrics.put("views", (long) i); + metrics.put("cpu", i * 0.5); + metrics.put("host", "host-" + i); + row.putValue(parent, metrics); + row.putValue("dim", "val-" + i); + rows.add(row); + } + SegmentIndexCreationDriverImpl driver = new SegmentIndexCreationDriverImpl(); + driver.init(config, new GenericRowRecordReader(rows)); + driver.build(); + return driver.getOutputDirectory(); + } } diff --git a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java index d79515ba9485..8ed3b65f2208 100644 --- a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java +++ b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java @@ -19,6 +19,7 @@ package org.apache.pinot.segment.spi.index.metadata; import com.fasterxml.jackson.core.type.TypeReference; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Maps; import it.unimi.dsi.fastutil.ints.IntSet; import java.math.BigDecimal; @@ -57,6 +58,14 @@ import org.apache.pinot.spi.utils.JsonUtils; +/// Column metadata parsed from `metadata.properties` (or built through [Builder]). +/// +/// A server retains one instance per (segment, column) for as long as the segment is loaded, so the parse path keeps +/// the per-column footprint small: column names, parent-column names, date-time formats/granularities and custom +/// default-null literals are interned (they recur in every segment of a table), and a `defaultNullValue` that equals +/// the type default is not handed to the [FieldSpec] at all, so the spec carries the shared static +/// `FieldSpec.DEFAULT_*` constant and never retains the literal. Segment-derived [FieldSpec]s must therefore be +/// treated as read-only: nothing may mutate their default null value in place (nothing ever did). @SuppressWarnings({"rawtypes", "unchecked"}) public class ColumnMetadataImpl implements ColumnMetadata { private static final long SIZE_MASK = 0xffffffffffffL; @@ -419,7 +428,7 @@ public static ColumnMetadataImpl fromPropertiesConfiguration(PropertiesConfigura .setMaxRowLengthInBytes(config.getInt(Column.getKeyFor(column, Column.MAX_ROW_LENGTH_IN_BYTES), UNAVAILABLE)) .setBitsPerElement(config.getInt(Column.getKeyFor(column, Column.BITS_PER_ELEMENT), UNAVAILABLE)) .setAutoGenerated(config.getBoolean(Column.getKeyFor(column, Column.IS_AUTO_GENERATED), false)) - .setParentColumn(config.getString(Column.getKeyFor(column, Column.PARENT_COLUMN), null)); + .setParentColumn(intern(config.getString(Column.getKeyFor(column, Column.PARENT_COLUMN), null))); Object rawSparseKeys = config.getProperty(Column.getKeyFor(column, Column.SPARSE_KEYS)); if (rawSparseKeys != null) { @@ -496,7 +505,11 @@ private static ChunkCompressionType parseCompressionType(String column, @Nullabl } public static FieldSpec extractFieldSpec(String column, PropertiesConfiguration config) { - String fieldName = config.getString(Column.getKeyFor(column, Column.COLUMN_NAME), column); + // The name is retained by the FieldSpec, the segment Schema and every per-segment column map, and it recurs in + // every segment of the table: intern it so all of them alias one JVM-wide instance. When COLUMN_NAME is absent + // (the segment creator only writes it when it differs from the key) this is the key parsed by SegmentMetadataImpl, + // which is already interned, so the lookup just returns it. + String fieldName = config.getString(Column.getKeyFor(column, Column.COLUMN_NAME), column).intern(); FieldType fieldType = config.getEnum(Column.getKeyFor(column, Column.COLUMN_TYPE), FieldType.class); DataType dataType = config.getEnum(Column.getKeyFor(column, Column.DATA_TYPE), DataType.class); boolean isSingleValue = config.getBoolean(Column.getKeyFor(column, Column.IS_SINGLE_VALUED), true); @@ -511,24 +524,26 @@ public static FieldSpec extractFieldSpec(String column, PropertiesConfiguration ? FieldSpec.MaxLengthExceedStrategy.valueOf(maxLengthExceedStrategyString) : null; switch (fieldType) { case DIMENSION: - return new DimensionFieldSpec(fieldName, dataType, isSingleValue, maxLength, defaultNullValueString, - maxLengthExceedStrategy); + return new DimensionFieldSpec(fieldName, dataType, isSingleValue, maxLength, + canonicalDefaultNullValue(fieldType, dataType, defaultNullValueString), maxLengthExceedStrategy); case METRIC: - return new MetricFieldSpec(fieldName, dataType, defaultNullValueString, maxLength, maxLengthExceedStrategy); + return new MetricFieldSpec(fieldName, dataType, + canonicalDefaultNullValue(fieldType, dataType, defaultNullValueString), maxLength, maxLengthExceedStrategy); case TIME: TimeUnit timeUnit = TimeUnit.valueOf(config.getString(Segment.TIME_UNIT, "DAYS").toUpperCase()); return new TimeFieldSpec(new TimeGranularitySpec(dataType, timeUnit, fieldName)); case DATE_TIME: - String format = config.getString(Column.getKeyFor(column, Column.DATETIME_FORMAT)); - String granularity = config.getString(Column.getKeyFor(column, Column.DATETIME_GRANULARITY)); - return new DateTimeFieldSpec(fieldName, dataType, format, granularity, defaultNullValueString, null); + String format = intern(config.getString(Column.getKeyFor(column, Column.DATETIME_FORMAT))); + String granularity = intern(config.getString(Column.getKeyFor(column, Column.DATETIME_GRANULARITY))); + return new DateTimeFieldSpec(fieldName, dataType, format, granularity, + canonicalDefaultNullValue(fieldType, dataType, defaultNullValueString), null); case COMPLEX: List childFieldNames = config.getList(String.class, Column.getKeyFor(column, Column.COMPLEX_CHILD_FIELD_NAMES)); Map childFieldSpecs = new HashMap<>(); if (childFieldNames != null) { for (String childField : childFieldNames) { - childFieldSpecs.put(childField, + childFieldSpecs.put(childField.intern(), extractFieldSpec(ComplexFieldSpec.getFullChildName(column, childField), config)); } } @@ -538,6 +553,37 @@ public static FieldSpec extractFieldSpec(String column, PropertiesConfiguration } } + /// Returns the `defaultNullValue` literal to hand to the [FieldSpec] constructor: `null` when the literal parses to + /// the type default, so the spec ends up holding the shared static `FieldSpec.DEFAULT_*` constant instead of a + /// per-segment box plus the literal (the segment creator writes the literal for every column, so without this every + /// column of every segment paid for it); otherwise the interned literal, so a custom default is shared across the + /// segments of the table. Equality is [DataType#equals(Object, Object)], the predicate [FieldSpec#equals] applies to + /// default null values, so the canonical spec equals one built from the literal and + /// [FieldSpec#getDefaultNullValueString()] (derived from the value) is unchanged; a BIG_DECIMAL literal with a + /// different scale or a negative-zero FLOAT/DOUBLE is not equal and stays verbatim. + @VisibleForTesting + @Nullable + static String canonicalDefaultNullValue(FieldType fieldType, DataType dataType, @Nullable String literal) { + if (literal == null) { + return null; + } + Object typeDefault; + try { + typeDefault = FieldSpec.getDefaultNullValue(fieldType, dataType, null); + } catch (IllegalStateException e) { + // No type default for this combination (e.g. a METRIC BOOLEAN): the literal is the only valid value, exactly as + // the FieldSpec constructor treats it. + return literal.intern(); + } + return dataType.equals(FieldSpec.getDefaultNullValue(fieldType, dataType, literal), typeDefault) ? null + : literal.intern(); + } + + @Nullable + private static String intern(@Nullable String value) { + return value != null ? value.intern() : null; + } + @Nullable public static PartitionFunction extractPartitionFunction(String column, PropertiesConfiguration config) { String partitionFunctionName = config.getString(Column.getKeyFor(column, Column.PARTITION_FUNCTION), null); diff --git a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java index 11e4ffd7c29d..b57434107b45 100644 --- a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java +++ b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java @@ -311,9 +311,14 @@ private static void setCustomConfigs(Configuration segmentMetadataPropertiesConf } /// Helper method to add the physical columns from source list to destination set. + /// + /// Column names are interned: the same names recur in every segment of a table and each one is retained by the + /// column metadata map key, the FieldSpec, the segment Schema and the loader's per-column maps, so one JVM-wide + /// instance replaces a copy per segment (the JVM string table holds them weakly, so they live exactly as long as a + /// loaded segment references them). private static void addPhysicalColumns(List src, Set dest) { for (Object o : src) { - String column = o.toString(); + String column = o.toString().intern(); if (!column.isEmpty() && !BuiltInVirtualColumn.BUILT_IN_VIRTUAL_COLUMNS.contains(column)) { // NOTE: // Exclude built in virtual columns. In regular case they shouldn't exist in the metadata file, but we perform diff --git a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java index 910e7080db71..44913ad14e95 100644 --- a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java +++ b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImplTest.java @@ -19,21 +19,32 @@ package org.apache.pinot.segment.spi.index.metadata; import com.fasterxml.jackson.databind.JsonNode; +import java.math.BigDecimal; +import java.util.Map; +import javax.annotation.Nullable; import org.apache.commons.configuration2.PropertiesConfiguration; import org.apache.pinot.segment.spi.ColumnMetadata; import org.apache.pinot.segment.spi.V1Constants.MetadataKeys.Column; import org.apache.pinot.segment.spi.compression.ChunkCompressionType; import org.apache.pinot.spi.config.table.FieldConfig.EncodingType; +import org.apache.pinot.spi.data.DateTimeFieldSpec; import org.apache.pinot.spi.data.DimensionFieldSpec; +import org.apache.pinot.spi.data.FieldSpec; import org.apache.pinot.spi.data.FieldSpec.DataType; import org.apache.pinot.spi.data.FieldSpec.FieldType; +import org.apache.pinot.spi.data.MetricFieldSpec; +import org.apache.pinot.spi.env.CommonsConfigurationUtils; +import org.apache.pinot.spi.utils.BytesUtils; import org.apache.pinot.spi.utils.JsonUtils; +import org.apache.pinot.spi.utils.UuidUtils; import org.testng.annotations.Test; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotEquals; +import static org.testng.Assert.assertNotSame; import static org.testng.Assert.assertNull; +import static org.testng.Assert.assertSame; import static org.testng.Assert.assertTrue; import static org.testng.Assert.expectThrows; @@ -266,9 +277,204 @@ public void rejectsInvalidIndexSize() { assertEquals(metadata.getNumIndexes(), 0, "a rejected size must not be recorded"); } + private static final String DATETIME_FORMAT = "1:MILLISECONDS:EPOCH"; + private static final String DATETIME_GRANULARITY = "1:MILLISECONDS"; + + /// The segment creator writes `defaultNullValue` for every column, so a column whose default is the type default + /// must come back holding the shared static constant (one instance per JVM instead of a box plus the literal per + /// segment) while staying equal to the spec the table schema would build. + @Test + public void typeDefaultLiteralSharesTheStaticConstant() { + Map dimensionDefaults = Map.ofEntries( + Map.entry(DataType.INT, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_INT), + Map.entry(DataType.LONG, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_LONG), + Map.entry(DataType.FLOAT, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_FLOAT), + Map.entry(DataType.DOUBLE, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_DOUBLE), + Map.entry(DataType.BOOLEAN, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_BOOLEAN), + Map.entry(DataType.TIMESTAMP, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_TIMESTAMP), + Map.entry(DataType.STRING, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_STRING), + Map.entry(DataType.JSON, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_JSON), + Map.entry(DataType.BYTES, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_BYTES), + Map.entry(DataType.BIG_DECIMAL, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_BIG_DECIMAL)); + dimensionDefaults.forEach((dataType, constant) -> { + FieldSpec spec = parse(FieldType.DIMENSION, dataType, writtenLiteral(dataType, constant)); + assertSame(spec.getDefaultNullValue(), constant, dataType.name()); + assertEquals(spec.getDefaultNullValueString(), dataType.toString(constant), dataType.name()); + assertEquals(spec, new DimensionFieldSpec("col", dataType, true), dataType.name()); + }); + + Map metricDefaults = Map.of( + DataType.INT, FieldSpec.DEFAULT_METRIC_NULL_VALUE_OF_INT, + DataType.LONG, FieldSpec.DEFAULT_METRIC_NULL_VALUE_OF_LONG, + DataType.FLOAT, FieldSpec.DEFAULT_METRIC_NULL_VALUE_OF_FLOAT, + DataType.DOUBLE, FieldSpec.DEFAULT_METRIC_NULL_VALUE_OF_DOUBLE, + DataType.BIG_DECIMAL, FieldSpec.DEFAULT_METRIC_NULL_VALUE_OF_BIG_DECIMAL, + DataType.STRING, FieldSpec.DEFAULT_METRIC_NULL_VALUE_OF_STRING, + DataType.BYTES, FieldSpec.DEFAULT_METRIC_NULL_VALUE_OF_BYTES); + metricDefaults.forEach((dataType, constant) -> { + FieldSpec spec = parse(FieldType.METRIC, dataType, writtenLiteral(dataType, constant)); + assertSame(spec.getDefaultNullValue(), constant, dataType.name()); + assertEquals(spec.getDefaultNullValueString(), dataType.toString(constant), dataType.name()); + assertEquals(spec, new MetricFieldSpec("col", dataType), dataType.name()); + }); + } + + /// The UUID default is a fresh nil-UUID array per lookup, so there is no constant to share; the literal is still + /// recognised as the type default (it is dropped rather than retained) and the value stays equal. + @Test + public void uuidTypeDefaultStaysValueEqual() { + String literal = UuidUtils.toString(UuidUtils.nullUuidBytes()); + assertNull(ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.DIMENSION, DataType.UUID, literal)); + FieldSpec spec = parse(FieldType.DIMENSION, DataType.UUID, literal); + assertEquals((byte[]) spec.getDefaultNullValue(), UuidUtils.nullUuidBytes()); + assertEquals(spec.getDefaultNullValueString(), literal); + assertEquals(spec, new DimensionFieldSpec("col", DataType.UUID, true)); + } + + /// Custom defaults are parsed exactly as before and equal the spec a table schema builds; the literal itself is + /// interned so the segments of a table share it. + @Test + public void customLiteralsRoundTrip() { + FieldSpec intSpec = parse(FieldType.DIMENSION, DataType.INT, "-1"); + assertEquals(intSpec, new DimensionFieldSpec("col", DataType.INT, true, -1)); + assertNotSame(intSpec.getDefaultNullValue(), FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_INT); + assertEquals(parse(FieldType.DIMENSION, DataType.STRING, "N/A"), + new DimensionFieldSpec("col", DataType.STRING, true, "N/A")); + assertEquals(parse(FieldType.DIMENSION, DataType.BYTES, "abcd"), + new DimensionFieldSpec("col", DataType.BYTES, true, BytesUtils.toBytes("abcd"))); + assertEquals(parse(FieldType.METRIC, DataType.DOUBLE, "1.5"), new MetricFieldSpec("col", DataType.DOUBLE, 1.5)); + // Equality is the data type's own: a BIG_DECIMAL with another scale and a negative zero are not the type default, + // so their string form survives the round trip. + FieldSpec scaledZero = parse(FieldType.DIMENSION, DataType.BIG_DECIMAL, "0.0"); + assertNotSame(scaledZero.getDefaultNullValue(), FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_BIG_DECIMAL); + assertEquals(scaledZero.getDefaultNullValueString(), "0.0"); + assertEquals(scaledZero, new DimensionFieldSpec("col", DataType.BIG_DECIMAL, true, new BigDecimal("0.0"))); + FieldSpec negativeZero = parse(FieldType.METRIC, DataType.FLOAT, "-0.0"); + assertNotSame(negativeZero.getDefaultNullValue(), FieldSpec.DEFAULT_METRIC_NULL_VALUE_OF_FLOAT); + assertEquals(negativeZero.getDefaultNullValueString(), "-0.0"); + + assertSame(ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.DIMENSION, DataType.INT, new String("-1")), + "-1"); + assertNull(ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.DIMENSION, DataType.INT, null)); + } + + /// A STRING default with a leading/trailing space or a comma is escaped by the segment creator and recovered here + /// before the type-default comparison, so it round-trips verbatim. + @Test + public void stringDefaultWithSpecialCharactersRoundTrips() { + String custom = " a,b "; + FieldSpec spec = parse(FieldType.DIMENSION, DataType.STRING, + CommonsConfigurationUtils.replaceSpecialCharacterInPropertyValue(custom)); + assertEquals(spec.getDefaultNullValue(), custom); + assertEquals(spec, new DimensionFieldSpec("col", DataType.STRING, true, custom)); + FieldSpec typeDefault = parse(FieldType.DIMENSION, DataType.STRING, + CommonsConfigurationUtils.replaceSpecialCharacterInPropertyValue( + FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_STRING)); + assertSame(typeDefault.getDefaultNullValue(), FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_STRING); + } + + @Test + public void dateTimeDefaultsAndFormatStrings() { + PropertiesConfiguration config = configFor(FieldType.DATE_TIME, DataType.LONG, + DataType.LONG.toString(FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_LONG)); + DateTimeFieldSpec spec = (DateTimeFieldSpec) ColumnMetadataImpl.extractFieldSpec("col", config); + assertSame(spec.getDefaultNullValue(), FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_LONG); + assertEquals(spec, new DateTimeFieldSpec("col", DataType.LONG, DATETIME_FORMAT, DATETIME_GRANULARITY)); + // The format and granularity are stored as fresh strings and come back as the interned instances. + assertSame(spec.getFormat(), DATETIME_FORMAT); + assertSame(spec.getGranularity(), DATETIME_GRANULARITY); + + DateTimeFieldSpec custom = + (DateTimeFieldSpec) ColumnMetadataImpl.extractFieldSpec("col", configFor(FieldType.DATE_TIME, DataType.LONG, + "0")); + assertEquals(custom, + new DateTimeFieldSpec("col", DataType.LONG, DATETIME_FORMAT, DATETIME_GRANULARITY, 0L, null)); + assertEquals(custom.getDefaultNullValue(), 0L); + } + + /// `/tables/{table}/segments/{segment}/metadata` bean-serializes the FieldSpec, i.e. `getDefaultNullValue()` by + /// value, so its payload is byte-identical to the one a spec built straight from the literal (the + /// pre-canonicalization shape) produces, for every type including BYTES and UUID. Only `FieldSpec#toJsonObject()`, + /// which compares the value against the type default by identity, now omits a redundant BYTES default that used to + /// be emitted; no endpoint serializes a segment-derived schema that way. + @Test + public void segmentMetadataJsonUnchangedByCanonicalization() + throws Exception { + for (DataType dataType : new DataType[] { + DataType.INT, DataType.LONG, DataType.FLOAT, DataType.DOUBLE, DataType.BOOLEAN, DataType.TIMESTAMP, + DataType.STRING, DataType.JSON, DataType.BYTES, DataType.UUID, DataType.BIG_DECIMAL + }) { + Object constant = FieldSpec.getDefaultNullValue(FieldType.DIMENSION, dataType, null); + String literal = dataType.toString(constant); + FieldSpec parsed = parse(FieldType.DIMENSION, dataType, writtenLiteral(dataType, constant)); + FieldSpec legacy = new DimensionFieldSpec("col", dataType, true, literal); + assertEquals(JsonUtils.objectToString(parsed), JsonUtils.objectToString(legacy), dataType.name()); + assertEquals(parsed, legacy, dataType.name()); + } + assertFalse(parse(FieldType.DIMENSION, DataType.BYTES, "").toJsonObject().has("defaultNullValue")); + } + + /// A combination with no type default (rejected by schema validation, but constructible with an explicit default) + /// keeps parsing the literal instead of failing on the type-default lookup. + @Test + public void literalWithoutTypeDefaultIsKept() { + assertSame(ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.METRIC, DataType.BOOLEAN, new String("1")), + "1"); + assertEquals(parse(FieldType.METRIC, DataType.BOOLEAN, "1").getDefaultNullValue(), 1); + } + + /// The strings a column retains for its lifetime alias the JVM-wide interned instances, so every segment of the + /// table shares them. + @Test + public void columnNameAndParentColumnAreInterned() { + PropertiesConfiguration config = baseConfig("metrics$cpu"); + config.setProperty(Column.getKeyFor("metrics$cpu", Column.COLUMN_NAME), new String("cpu")); + config.setProperty(Column.getKeyFor("metrics$cpu", Column.PARENT_COLUMN), new String("metrics")); + + ColumnMetadataImpl metadata = ColumnMetadataImpl.fromPropertiesConfiguration(config, 1, "metrics$cpu"); + + assertSame(metadata.getFieldSpec().getName(), "cpu"); + assertSame(metadata.getParentColumn(), "metrics"); + // Without an explicit COLUMN_NAME the key itself is the name. + String column = new String("plain"); + FieldSpec spec = ColumnMetadataImpl.extractFieldSpec(column, baseConfigWithoutName(column)); + assertSame(spec.getName(), "plain"); + } + + private static FieldSpec parse(FieldType fieldType, DataType dataType, @Nullable String defaultNullValue) { + return ColumnMetadataImpl.extractFieldSpec("col", configFor(fieldType, dataType, defaultNullValue)); + } + + /// The literal the segment creator writes for the given default null value. + private static String writtenLiteral(DataType dataType, Object defaultNullValue) { + String literal = dataType.toString(defaultNullValue); + return dataType.getStoredType() == DataType.STRING + ? CommonsConfigurationUtils.replaceSpecialCharacterInPropertyValue(literal) : literal; + } + + private static PropertiesConfiguration configFor(FieldType fieldType, DataType dataType, + @Nullable String defaultNullValue) { + PropertiesConfiguration config = baseConfig("col"); + config.setProperty(Column.getKeyFor("col", Column.COLUMN_TYPE), fieldType.name()); + config.setProperty(Column.getKeyFor("col", Column.DATA_TYPE), dataType.name()); + if (defaultNullValue != null) { + config.setProperty(Column.getKeyFor("col", Column.DEFAULT_NULL_VALUE), defaultNullValue); + } + if (fieldType == FieldType.DATE_TIME) { + config.setProperty(Column.getKeyFor("col", Column.DATETIME_FORMAT), new String(DATETIME_FORMAT)); + config.setProperty(Column.getKeyFor("col", Column.DATETIME_GRANULARITY), new String(DATETIME_GRANULARITY)); + } + return config; + } + private static PropertiesConfiguration baseConfig(String column) { - PropertiesConfiguration config = new PropertiesConfiguration(); + PropertiesConfiguration config = baseConfigWithoutName(column); config.setProperty(Column.getKeyFor(column, Column.COLUMN_NAME), column); + return config; + } + + private static PropertiesConfiguration baseConfigWithoutName(String column) { + PropertiesConfiguration config = new PropertiesConfiguration(); config.setProperty(Column.getKeyFor(column, Column.COLUMN_TYPE), FieldType.DIMENSION.name()); config.setProperty(Column.getKeyFor(column, Column.DATA_TYPE), DataType.STRING.name()); config.setProperty(Column.getKeyFor(column, Column.IS_SINGLE_VALUED), true);