This is an automated email from the ASF dual-hosted git repository.
xiangfu0 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new e1762ad216d Use dedicated weak interners for metadata column names and
defaults (#19584)
e1762ad216d is described below
commit e1762ad216dbe5db000a64fc7cebd66b6dec526c
Author: Xiang Fu <[email protected]>
AuthorDate: Thu Sep 17 14:56:41 2026 -0700
Use dedicated weak interners for metadata column names and defaults (#19584)
* Use dedicated weak interners for metadata column names and defaults
* Place the column interner constructor next to the class declaration
---
.../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..06f8bdf4a55
--- /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 ColumnNameInterner() {
+ }
+
+ private static final Interner<String> INTERNER = Interners.newWeakInterner();
+
+ @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]
