This is an automated email from the ASF dual-hosted git repository. xiangfu0 pushed a commit to branch xiangfu0/codex/metadata-string-interners in repository https://gitbox.apache.org/repos/asf/pinot.git
commit 8b12bd36cc22e9fe5d7cbdb649906245ad161e68 Author: Xiang Fu <[email protected]> AuthorDate: Wed Sep 16 18:21:56 2026 -0700 Use dedicated weak interners for metadata column names and defaults --- .../spi/index/metadata/ColumnMetadataImpl.java | 16 +++-- .../spi/index/metadata/SegmentMetadataImpl.java | 10 +-- .../spi/index/metadata/ColumnMetadataImplTest.java | 78 ++++++++++++++++++++-- .../apache/pinot/spi/utils/ColumnNameInterner.java | 38 +++++++++++ 4 files changed, 124 insertions(+), 18 deletions(-) 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 747c6d8b14f..4e3968125b5 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 @@ -57,6 +57,7 @@ import org.apache.pinot.spi.data.TimeFieldSpec; import org.apache.pinot.spi.data.TimeGranularitySpec; import org.apache.pinot.spi.env.CommonsConfigurationUtils; import org.apache.pinot.spi.utils.BytesUtils; +import org.apache.pinot.spi.utils.ColumnNameInterner; import org.apache.pinot.spi.utils.JsonUtils; import static com.google.common.base.Preconditions.checkElementIndex; @@ -82,6 +83,7 @@ public class ColumnMetadataImpl implements ColumnMetadata { /// are held weakly: the canonical instance is exactly the one the loaded segments retain, so it lives as long as /// any of them and is released once the last one is unloaded. Thread-safe. private static final Interner<FieldSpec> FIELD_SPEC_INTERNER = Interners.newWeakInterner(); + private static final Interner<String> DEFAULT_NULL_VALUE_INTERNER = Interners.newWeakInterner(); private final FieldSpec _fieldSpec; private final int _totalDocs; @@ -435,7 +437,8 @@ public class ColumnMetadataImpl implements ColumnMetadata { .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(intern(config.getString(Column.getKeyFor(column, Column.PARENT_COLUMN), null))); + .setParentColumn( + ColumnNameInterner.intern(config.getString(Column.getKeyFor(column, Column.PARENT_COLUMN), null))); Object rawSparseKeys = config.getProperty(Column.getKeyFor(column, Column.SPARSE_KEYS)); if (rawSparseKeys != null) { @@ -519,10 +522,11 @@ public class ColumnMetadataImpl implements ColumnMetadata { @SuppressWarnings("deprecation") // Preserve the field type when loading legacy TIME column metadata. public static FieldSpec extractFieldSpec(String column, PropertiesConfiguration config) { // 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 + // every segment of the table: share it through the column-name interner. 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(); + String fieldName = + ColumnNameInterner.intern(config.getString(Column.getKeyFor(column, Column.COLUMN_NAME), column)); 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); @@ -557,7 +561,7 @@ public class ColumnMetadataImpl implements ColumnMetadata { Map<String, FieldSpec> childFieldSpecs = new HashMap<>(); if (childFieldNames != null) { for (String childField : childFieldNames) { - childFieldSpecs.put(childField.intern(), + childFieldSpecs.put(ColumnNameInterner.intern(childField), extractFieldSpec(ComplexFieldSpec.getFullChildName(column, childField), config)); } } @@ -588,10 +592,10 @@ public class ColumnMetadataImpl implements ColumnMetadata { } 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 DEFAULT_NULL_VALUE_INTERNER.intern(literal); } return dataType.equals(FieldSpec.getDefaultNullValue(fieldType, dataType, literal), typeDefault) ? null - : literal.intern(); + : DEFAULT_NULL_VALUE_INTERNER.intern(literal); } @Nullable 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 7a09ad5243b..31f72dc6506 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 @@ -59,6 +59,7 @@ import org.apache.pinot.segment.spi.store.SegmentDirectoryPaths; import org.apache.pinot.segment.spi.utils.SegmentMetadataUtils; import org.apache.pinot.spi.data.Schema; import org.apache.pinot.spi.env.CommonsConfigurationUtils; +import org.apache.pinot.spi.utils.ColumnNameInterner; import org.apache.pinot.spi.utils.CommonConstants.Segment.BuiltInVirtualColumn; import org.apache.pinot.spi.utils.JsonUtils; import org.apache.pinot.spi.utils.TimeUtils; @@ -160,7 +161,7 @@ public class SegmentMetadataImpl implements SegmentMetadata { /// - Start and End time. private void setTimeInfo(PropertiesConfiguration segmentMetadataPropertiesConfiguration) { String timeColumn = segmentMetadataPropertiesConfiguration.getString(Segment.TIME_COLUMN_NAME); - _timeColumn = timeColumn != null ? timeColumn.intern() : null; + _timeColumn = ColumnNameInterner.intern(timeColumn); if (segmentMetadataPropertiesConfiguration.containsKey(Segment.SEGMENT_START_TIME) && segmentMetadataPropertiesConfiguration.containsKey(Segment.SEGMENT_END_TIME) && segmentMetadataPropertiesConfiguration.containsKey(Segment.TIME_UNIT)) { @@ -311,12 +312,11 @@ public class SegmentMetadataImpl implements SegmentMetadata { /// 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). + /// column metadata map key, the FieldSpec, the segment Schema and the loader's per-column maps. + /// [ColumnNameInterner] holds these shared names weakly so unused names can be reclaimed. private static void addPhysicalColumns(List<Object> src, Set<String> dest) { for (Object o : src) { - String column = o.toString().intern(); + String column = ColumnNameInterner.intern(o.toString()); 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 28cca6fb846..5b017e6fd7c 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 @@ -20,9 +20,15 @@ package org.apache.pinot.segment.spi.index.metadata; import com.fasterxml.jackson.databind.JsonNode; import java.math.BigDecimal; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; import org.apache.commons.configuration2.PropertiesConfiguration; @@ -43,6 +49,7 @@ import org.apache.pinot.spi.data.TimeFieldSpec; import org.apache.pinot.spi.data.TimeGranularitySpec; import org.apache.pinot.spi.env.CommonsConfigurationUtils; import org.apache.pinot.spi.utils.BytesUtils; +import org.apache.pinot.spi.utils.ColumnNameInterner; import org.apache.pinot.spi.utils.JsonUtils; import org.apache.pinot.spi.utils.UuidUtils; import org.testng.annotations.Test; @@ -382,8 +389,10 @@ public class ColumnMetadataImplTest { assertNotSame(negativeZero.getDefaultNullValue(), FieldSpec.DEFAULT_METRIC_NULL_VALUE_OF_FLOAT); assertEquals(negativeZero.getDefaultNullValueString(), "-0.0"); + String literal = ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.DIMENSION, DataType.INT, new String("-1")); + assertEquals(literal, "-1"); assertSame(ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.DIMENSION, DataType.INT, new String("-1")), - "-1"); + literal); assertNull(ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.DIMENSION, DataType.INT, null)); } @@ -463,13 +472,15 @@ public class ColumnMetadataImplTest { /// keeps parsing the literal instead of failing on the type-default lookup. @Test public void literalWithoutTypeDefaultIsKept() { + String literal = + ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.METRIC, DataType.BOOLEAN, new String("1")); + assertEquals(literal, "1"); assertSame(ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.METRIC, DataType.BOOLEAN, new String("1")), - "1"); + literal); 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. + /// Column names and parent names share the same interner across metadata loads. @Test public void columnNameAndParentColumnAreInterned() { PropertiesConfiguration config = baseConfig("metrics$cpu"); @@ -478,12 +489,65 @@ public class ColumnMetadataImplTest { ColumnMetadataImpl metadata = ColumnMetadataImpl.fromPropertiesConfiguration(config, 1, "metrics$cpu"); - assertSame(metadata.getFieldSpec().getName(), "cpu"); - assertSame(metadata.getParentColumn(), "metrics"); + assertEquals(metadata.getFieldSpec().getName(), "cpu"); + assertSame(metadata.getFieldSpec().getName(), ColumnNameInterner.intern(new String("cpu"))); + assertEquals(metadata.getParentColumn(), "metrics"); + assertSame(metadata.getParentColumn(), ColumnNameInterner.intern(new String("metrics"))); + assertNull(ColumnNameInterner.intern(null)); // 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"); + assertEquals(spec.getName(), "plain"); + assertSame(spec.getName(), ColumnNameInterner.intern(new String("plain"))); + } + + @Test + public void columnNamesAndDefaultsUseSeparateInterners() { + String value = "metadata_" + UUID.randomUUID(); + String pooled = new String(value).intern(); + String name = ColumnNameInterner.intern(new String(value)); + String defaultValue = + ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.DIMENSION, DataType.STRING, new String(value)); + assertEquals(name, value); + assertEquals(defaultValue, value); + assertNotSame(name, pooled); + assertNotSame(defaultValue, pooled); + assertNotSame(name, defaultValue); + assertSame(ColumnNameInterner.intern(new String(value)), name); + assertSame(ColumnMetadataImpl.canonicalDefaultNullValue(FieldType.DIMENSION, DataType.STRING, new String(value)), + defaultValue); + } + + @Test + public void columnNamesAndDefaultsAreSharedDuringConcurrentParsing() + throws Exception { + ExecutorService executor = Executors.newFixedThreadPool(8); + CountDownLatch start = new CountDownLatch(1); + try { + List<Future<FieldSpec>> results = new ArrayList<>(); + for (int i = 0; i < 32; i++) { + int maxLength = 100 + i; + results.add(executor.submit(() -> { + assertTrue(start.await(10, TimeUnit.SECONDS)); + PropertiesConfiguration config = + configFor(FieldType.DIMENSION, DataType.STRING, new String("shared-default")); + config.setProperty(Column.getKeyFor("col", Column.COLUMN_NAME), new String("shared-column")); + config.setProperty(Column.getKeyFor("col", Column.SCHEMA_MAX_LENGTH), maxLength); + return ColumnMetadataImpl.extractFieldSpec("col", config); + })); + } + start.countDown(); + FieldSpec first = results.get(0).get(10, TimeUnit.SECONDS); + for (int i = 1; i < results.size(); i++) { + FieldSpec other = results.get(i).get(10, TimeUnit.SECONDS); + assertNotSame(other, first, "Different max lengths must not share a FieldSpec"); + assertSame(other.getName(), first.getName()); + assertSame(other.getDefaultNullValue(), first.getDefaultNullValue()); + } + } finally { + start.countDown(); + executor.shutdownNow(); + } } /// A server retains one FieldSpec per (segment, column) and every segment of a table parses the same column diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/ColumnNameInterner.java b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/ColumnNameInterner.java new file mode 100644 index 00000000000..33f68fc8d10 --- /dev/null +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/ColumnNameInterner.java @@ -0,0 +1,38 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.spi.utils; + +import com.google.common.collect.Interner; +import com.google.common.collect.Interners; +import javax.annotation.Nullable; + + +/// Shares equal column names across metadata readers without using the JVM string pool. +/// The interner is thread-safe and holds names weakly, allowing unused names to be reclaimed. +public final class ColumnNameInterner { + private static final Interner<String> INTERNER = Interners.newWeakInterner(); + + private ColumnNameInterner() { + } + + @Nullable + public static String intern(@Nullable String columnName) { + return columnName != null ? INTERNER.intern(columnName) : null; + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
