From 289f83918e5fb69c92fc78a2d42f72912331fd96 Mon Sep 17 00:00:00 2001 From: Xiang Fu Date: Sat, 5 Sep 2026 18:04:57 -0700 Subject: [PATCH] Slim ColumnMetadataImpl to 72 bytes and derive the per-segment Schema lazily A server retains one ColumnMetadataImpl and one Schema entry per (segment, column) for as long as a segment is loaded. On a server holding tens of thousands of wide (1000+ column) segments that per-column retention dominates the heap, and a segment-level heap benchmark attributes ~100 B/col to ColumnMetadataImpl plus ~50 B/col (a TreeMap entry and list slack) to the eagerly built segment Schema, which duplicates the FieldSpecs the column metadata already holds. ColumnMetadataImpl: the six booleans and the forward-index encoding are packed into one flags byte, and the four refs only a partitioned column or an OPEN_STRUCT parent/child carries (partition function, partitions, parent column, sparse keys) move into a lazily allocated Extras holder that stays null for ordinary columns. CompressionMetadata stays a direct ref because the segment creator writes compression stats for every raw column, so a holder hop would be net negative there. 96 -> 72 bytes for an ordinary column; every public getter, the Builder, equals/hashCode and the toString field names are unchanged. SegmentMetadataImpl: the Schema is no longer built in init(). getSchema() derives it from the column metadata map on first use (double-checked on a volatile, so one instance per metadata) and caches it; removeColumn() drops the cache; the explicit-Schema constructor used by consuming segments returns the caller's Schema as before. getAllColumns() is overridden to the map's key set so the index handlers, index directories, BaseTableDataManager.isSegmentStale and TablesResource never materialize a Schema. toJson() reads the cached schema name without building one. Load and query paths move off getSchema(): ImmutableSegmentLoader registers the built-in virtual columns by building their FieldSpecs straight from BuiltInVirtualColumnDefinitions (new VirtualColumnProviderFactory .createBuiltInFieldSpec, which addBuiltInVirtualColumnsToSegmentSchema now delegates to, so RealtimeTableDataManager is unchanged); ImmutableSegmentImpl and EmptyIndexSegment serve getColumnNames()/getPhysicalColumnNames() as unmodifiable views of the column metadata map (same sorted order as the Schema keys, so SELECT * ordering is unchanged, and no per-column retention) and look up the OPEN_STRUCT parent ComplexFieldSpec in the column metadata; PinotSegmentColumnReaderImpl takes the data type from the column's data source, which also covers mutable segments. Compatibility: no public signature changes. getSchema() still returns an equal Schema, including the built-in virtual columns after load, only built later (Schema.equals is order-insensitive; dimension/metric list order becomes sorted instead of HashSet order). The segment-metadata REST JSON is unchanged: the ColumnMetadataImpl getter set is identical and the flags byte and Extras holder are private, pinned by a JSON key-set snapshot test. No on-disk format change; everything is server-local. Preprocess-time callers (ForwardIndexHandler, ColumnMinMaxValueGenerator, StarTreeV2BuilderConfig) and tools may still call getSchema() freely: SegmentPreProcessor reloads the metadata it worked on before the segment is served. A test-visible materialization counter and isSchemaMaterialized() guard the contract: LoaderTest asserts the served segment's schema is still unbuilt after ImmutableSegmentLoader.load, and SegmentMetadataImplTest asserts the counter is unchanged across a load plus column listings, data sources and toJson, so a stray hot-path getSchema() caller fails CI instead of silently re-inflating every queried segment. Co-Authored-By: Claude Fable 5.1 --- .../immutable/EmptyIndexSegment.java | 7 +- .../immutable/ImmutableSegmentImpl.java | 65 +++--- .../immutable/ImmutableSegmentLoader.java | 28 ++- .../immutable/PhysicalColumnNames.java | 103 ++++++++ .../readers/PinotSegmentColumnReaderImpl.java | 5 +- .../VirtualColumnProviderFactory.java | 34 +-- .../immutable/EmptyIndexSegmentTest.java | 36 +++ .../immutable/ImmutableSegmentImplTest.java | 120 +++++++--- .../index/SegmentMetadataImplTest.java | 125 ++++++++++ .../segment/index/loader/LoaderTest.java | 6 + .../pinot/segment/spi/SegmentMetadata.java | 4 + .../index/metadata/ColumnMetadataImpl.java | 209 +++++++++++------ .../index/metadata/SegmentMetadataImpl.java | 100 ++++++-- .../metadata/ColumnMetadataImplTest.java | 220 ++++++++++++++++++ 14 files changed, 884 insertions(+), 178 deletions(-) create mode 100644 pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/PhysicalColumnNames.java diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/EmptyIndexSegment.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/EmptyIndexSegment.java index 0c3c3f877f08..8d99954ff548 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/EmptyIndexSegment.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/EmptyIndexSegment.java @@ -19,6 +19,7 @@ package org.apache.pinot.segment.local.indexsegment.immutable; import com.google.common.base.Preconditions; +import java.util.Collections; import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; @@ -79,14 +80,16 @@ public SegmentMetadataImpl getSegmentMetadata() { return _segmentMetadata; } + // Both are views of the column metadata map, so neither builds the segment schema (see SegmentMetadataImpl) + @Override public Set getColumnNames() { - return _segmentMetadata.getSchema().getColumnNames(); + return Collections.unmodifiableSet(_segmentMetadata.getColumnMetadataMap().keySet()); } @Override public Set getPhysicalColumnNames() { - return _segmentMetadata.getSchema().getPhysicalColumnNames(); + return new PhysicalColumnNames(_segmentMetadata.getColumnMetadataMap()); } @Override diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java index b295cf68a3c3..7d29a1a9d1d0 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java @@ -25,11 +25,13 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeMap; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicBoolean; @@ -111,6 +113,10 @@ public class ImmutableSegmentImpl implements ImmutableSegment { private final StarTreeIndexContainer _starTreeIndexContainer; private final TextIndexReader _multiColumnTextIndex; private final Map _dataSources; + // Views of the column metadata map's keys (all columns / the physical ones), so listing columns never builds the + // segment schema, which SegmentMetadataImpl derives on demand and a wide segment must not retain per column + private final Set _columnNames; + private final Set _physicalColumnNames; // Lazy column materialization; all null in eager mode. See the class documentation. @Nullable @@ -150,14 +156,16 @@ public ImmutableSegmentImpl( _columnMaterializer = null; _openStructChildren = null; _materializationLock = null; - _dataSources = - new Object2ObjectOpenHashMap<>(segmentMetadata.getColumnMetadataMap().size()); + TreeMap columnMetadataMap = segmentMetadata.getColumnMetadataMap(); + _columnNames = Collections.unmodifiableSet(columnMetadataMap.keySet()); + _physicalColumnNames = new PhysicalColumnNames(columnMetadataMap); + _dataSources = new Object2ObjectOpenHashMap<>(columnMetadataMap.size()); Map> openStructDenseChildren = new HashMap<>(); Map openStructSparseChildren = new HashMap<>(); Set openStructParents = new HashSet<>(); - for (Map.Entry entry : segmentMetadata.getColumnMetadataMap().entrySet()) { + for (Map.Entry entry : columnMetadataMap.entrySet()) { String colName = entry.getKey(); ColumnMetadata columnMetadata = entry.getValue(); @@ -181,20 +189,18 @@ public ImmutableSegmentImpl( } } - if (!openStructParents.isEmpty()) { - Schema schema = segmentMetadata.getSchema(); - for (String parent : openStructParents) { - FieldSpec fieldSpec = schema != null ? schema.getFieldSpecFor(parent) : null; - if (!(fieldSpec instanceof ComplexFieldSpec)) { - continue; - } - ColumnMetadata parentMetadata = segmentMetadata.getColumnMetadataMap().get(parent); - List sparseKeys = - parentMetadata instanceof ColumnMetadataImpl impl ? impl.getSparseKeys() : null; - _dataSources.put(parent, new ImmutableOpenStructDataSource((ComplexFieldSpec) fieldSpec, - openStructDenseChildren.getOrDefault(parent, Map.of()), - openStructSparseChildren.get(parent), segmentMetadata.getTotalDocs(), sparseKeys)); + for (String parent : openStructParents) { + // The parent's spec comes from its column metadata, not from the segment schema (see _columnNames) + ColumnMetadata parentMetadata = columnMetadataMap.get(parent); + FieldSpec fieldSpec = parentMetadata != null ? parentMetadata.getFieldSpec() : null; + if (!(fieldSpec instanceof ComplexFieldSpec)) { + continue; } + List sparseKeys = + parentMetadata instanceof ColumnMetadataImpl impl ? impl.getSparseKeys() : null; + _dataSources.put(parent, new ImmutableOpenStructDataSource((ComplexFieldSpec) fieldSpec, + openStructDenseChildren.getOrDefault(parent, Map.of()), + openStructSparseChildren.get(parent), segmentMetadata.getTotalDocs(), sparseKeys)); } _multiColumnTextIndex = multiColumnTextIndex; @@ -225,18 +231,22 @@ public ImmutableSegmentImpl( _columnMaterializer = columnMaterializer; _openStructChildren = groupOpenStructChildren(segmentMetadata); _materializationLock = new ReentrantReadWriteLock(); + TreeMap columnMetadataMap = segmentMetadata.getColumnMetadataMap(); + _columnNames = Collections.unmodifiableSet(columnMetadataMap.keySet()); + _physicalColumnNames = new PhysicalColumnNames(columnMetadataMap); _dataSources = new ConcurrentHashMap<>(); for (String column : materializedIndexContainers.keySet()) { materializeDataSource(column); } } - /// Groups the materialized OPEN_STRUCT child columns under their parent, keeping only the parents the segment schema - /// declares as complex (the same rule the eager constructor applies). + /// Groups the materialized OPEN_STRUCT child columns under their parent, keeping only the parents whose column + /// metadata declares them complex (the same rule the eager constructor applies). @Nullable private static Map> groupOpenStructChildren(SegmentMetadataImpl segmentMetadata) { Map> children = null; - for (Map.Entry entry : segmentMetadata.getColumnMetadataMap().entrySet()) { + Map columnMetadataMap = segmentMetadata.getColumnMetadataMap(); + for (Map.Entry entry : columnMetadataMap.entrySet()) { if (entry.getValue() instanceof ColumnMetadataImpl impl && impl.isMaterializedChild()) { if (children == null) { children = new HashMap<>(); @@ -247,9 +257,10 @@ private static Map> groupOpenStructChildren(SegmentMetadata if (children == null) { return null; } - Schema schema = segmentMetadata.getSchema(); - children.keySet() - .removeIf(parent -> !(schema != null && schema.getFieldSpecFor(parent) instanceof ComplexFieldSpec)); + children.keySet().removeIf(parent -> { + ColumnMetadata parentMetadata = columnMetadataMap.get(parent); + return parentMetadata == null || !(parentMetadata.getFieldSpec() instanceof ComplexFieldSpec); + }); return children.isEmpty() ? null : children; } @@ -296,9 +307,9 @@ private DataSource createOpenStructDataSource(String parent) { denseChildren.put(OpenStructNaming.parseKey(child), childDataSource); } } - ComplexFieldSpec fieldSpec = (ComplexFieldSpec) _segmentMetadata.getSchema().getFieldSpecFor(parent); - List sparseKeys = - columnMetadataMap.get(parent) instanceof ColumnMetadataImpl impl ? impl.getSparseKeys() : null; + ColumnMetadata parentMetadata = columnMetadataMap.get(parent); + ComplexFieldSpec fieldSpec = (ComplexFieldSpec) parentMetadata.getFieldSpec(); + List sparseKeys = parentMetadata instanceof ColumnMetadataImpl impl ? impl.getSparseKeys() : null; return new ImmutableOpenStructDataSource(fieldSpec, denseChildren, sparseChild, _segmentMetadata.getTotalDocs(), sparseKeys); } @@ -470,12 +481,12 @@ public DataSource getDataSource(String column, Schema schema) { @Override public Set getColumnNames() { - return _segmentMetadata.getSchema().getColumnNames(); + return _columnNames; } @Override public Set getPhysicalColumnNames() { - return _segmentMetadata.getSchema().getPhysicalColumnNames(); + return _physicalColumnNames; } @Override diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java index e731fb82a561..8f980ebd13a2 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java @@ -50,6 +50,7 @@ import org.apache.pinot.segment.spi.loader.SegmentDirectoryLoaderRegistry; import org.apache.pinot.segment.spi.store.SegmentDirectory; import org.apache.pinot.segment.spi.store.SegmentDirectoryPaths; +import org.apache.pinot.spi.data.BuiltInVirtualColumnDefinitions; import org.apache.pinot.spi.data.FieldSpec; import org.apache.pinot.spi.data.OpenStructNaming; import org.apache.pinot.spi.data.Schema; @@ -311,21 +312,26 @@ private static ImmutableSegmentImpl loadWithLazyColumns(SegmentDirectory segment starTreeIndexContainer, mcTextReader); } - /// Adds the built-in virtual columns to the segment schema and creates their index containers and metadata. + /// Creates the index containers and column metadata of the built-in virtual columns and registers them in the + /// segment metadata. Registering the metadata is what makes the segment schema include the virtual columns: the + /// schema is derived from the column metadata map on demand ([SegmentMetadataImpl#getSchema()]) and is deliberately + /// not built here, so a loaded segment retains no per-column schema entries until something asks for its schema. + /// A physical column of the same name wins, as in the schema-based registration this replaces. private static void instantiateVirtualColumns(SegmentMetadataImpl segmentMetadata, Map indexContainerMap) { Map columnMetadataMap = segmentMetadata.getColumnMetadataMap(); - Schema segmentSchema = segmentMetadata.getSchema(); - VirtualColumnProviderFactory.addBuiltInVirtualColumnsToSegmentSchema(segmentSchema, segmentMetadata.getName()); - for (FieldSpec fieldSpec : segmentSchema.getAllFieldSpecs()) { - if (fieldSpec.isVirtualColumn()) { - String columnName = fieldSpec.getName(); - VirtualColumnContext context = - new VirtualColumnContext(fieldSpec, segmentMetadata.getTotalDocs(), segmentMetadata); - VirtualColumnProvider provider = VirtualColumnProviderFactory.buildProvider(context); - indexContainerMap.put(columnName, provider.buildColumnIndexContainer(context)); - columnMetadataMap.put(columnName, provider.buildMetadata(context)); + String segmentName = segmentMetadata.getName(); + for (BuiltInVirtualColumnDefinitions.Definition definition : BuiltInVirtualColumnDefinitions.DEFINITIONS) { + String columnName = definition.getName(); + if (columnMetadataMap.containsKey(columnName)) { + continue; } + FieldSpec fieldSpec = VirtualColumnProviderFactory.createBuiltInFieldSpec(definition, segmentName); + VirtualColumnContext context = + new VirtualColumnContext(fieldSpec, segmentMetadata.getTotalDocs(), segmentMetadata); + VirtualColumnProvider provider = VirtualColumnProviderFactory.buildProvider(context); + indexContainerMap.put(columnName, provider.buildColumnIndexContainer(context)); + columnMetadataMap.put(columnName, provider.buildMetadata(context)); } } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/PhysicalColumnNames.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/PhysicalColumnNames.java new file mode 100644 index 000000000000..b9dce7f3b868 --- /dev/null +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/PhysicalColumnNames.java @@ -0,0 +1,103 @@ +/** + * 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.segment.local.indexsegment.immutable; + +import java.util.AbstractSet; +import java.util.Iterator; +import java.util.Map; +import java.util.NoSuchElementException; +import java.util.SortedMap; +import org.apache.pinot.segment.spi.ColumnMetadata; + + +/// Unmodifiable view of the physical columns of an immutable segment: the keys of its column metadata map whose field +/// spec is not produced by a virtual column provider, in the map's (sorted) key order. +/// +/// It is a view rather than a copy so a segment retains nothing per column for it: the segment schema this replaces +/// held a `TreeMap` entry per column, and a cached `TreeSet` would hold the same. `contains` is one map lookup and +/// iteration is a filtered pass over the map. The virtual column count is taken once at construction, which is sound +/// because the column metadata map is fixed once the segment is loaded. +/// +/// Thread-safe for reads, like the underlying map once loaded. +final class PhysicalColumnNames extends AbstractSet { + private final SortedMap _columnMetadataMap; + private final int _numVirtualColumns; + + PhysicalColumnNames(SortedMap columnMetadataMap) { + _columnMetadataMap = columnMetadataMap; + int numVirtualColumns = 0; + for (ColumnMetadata columnMetadata : columnMetadataMap.values()) { + if (!isPhysical(columnMetadata)) { + numVirtualColumns++; + } + } + _numVirtualColumns = numVirtualColumns; + } + + private static boolean isPhysical(ColumnMetadata columnMetadata) { + return !columnMetadata.getFieldSpec().isVirtualColumn(); + } + + @Override + public boolean contains(Object o) { + if (!(o instanceof String)) { + return false; + } + ColumnMetadata columnMetadata = _columnMetadataMap.get(o); + return columnMetadata != null && isPhysical(columnMetadata); + } + + @Override + public int size() { + return _columnMetadataMap.size() - _numVirtualColumns; + } + + @Override + public Iterator iterator() { + Iterator> entries = _columnMetadataMap.entrySet().iterator(); + return new Iterator<>() { + private String _next = advance(); + + private String advance() { + while (entries.hasNext()) { + Map.Entry entry = entries.next(); + if (isPhysical(entry.getValue())) { + return entry.getKey(); + } + } + return null; + } + + @Override + public boolean hasNext() { + return _next != null; + } + + @Override + public String next() { + String next = _next; + if (next == null) { + throw new NoSuchElementException(); + } + _next = advance(); + return next; + } + }; + } +} diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImpl.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImpl.java index a28ed0f82fa2..293942b7be8d 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImpl.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImpl.java @@ -59,10 +59,11 @@ public PinotSegmentColumnReaderImpl(IndexSegment indexSegment, String columnName /// the segment's stored value (which contains the default). public PinotSegmentColumnReaderImpl(IndexSegment indexSegment, String columnName, boolean skipDefaultNullValues) { + // The data source's field spec is the column's own, so this never builds the segment schema (which an immutable + // segment derives on demand) and also covers a mutable segment, whose metadata has no column metadata map this(new PinotSegmentColumnReader(indexSegment, columnName), columnName, indexSegment.getSegmentMetadata().getTotalDocs(), - indexSegment.getSegmentMetadata().getSchema().getFieldSpecFor(columnName).getDataType(), - skipDefaultNullValues); + indexSegment.getDataSource(columnName).getDataSourceMetadata().getDataType(), skipDefaultNullValues); } /// Constructor for subclasses that need to provide their own PinotSegmentColumnReader. diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/virtualcolumn/VirtualColumnProviderFactory.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/virtualcolumn/VirtualColumnProviderFactory.java index 4d9f04908c31..c6dfaa2cfd8b 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/virtualcolumn/VirtualColumnProviderFactory.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/virtualcolumn/VirtualColumnProviderFactory.java @@ -86,26 +86,32 @@ public static VirtualColumnProvider buildProvider(VirtualColumnContext virtualCo /// [BuiltInVirtualColumnDefinitions#DEFINITIONS], which the broker side uses as well, so the two can never /// disagree on a type. /// This method only layers on the provider class, and the constant value for the columns whose value is already - /// known here. + /// known here (see [#createBuiltInFieldSpec(BuiltInVirtualColumnDefinitions.Definition, String)]). public static void addBuiltInVirtualColumnsToSegmentSchema(Schema schema, String segmentName) { for (BuiltInVirtualColumnDefinitions.Definition definition : BuiltInVirtualColumnDefinitions.DEFINITIONS) { - String column = definition.getName(); - if (schema.hasColumn(column)) { - continue; + if (!schema.hasColumn(definition.getName())) { + schema.addField(createBuiltInFieldSpec(definition, segmentName)); } - DimensionFieldSpec fieldSpec = definition.createFieldSpec(); - fieldSpec.setVirtualColumnProvider(getProviderClass(column).getName()); - // $hostName and $segmentName are constants known at schema construction time, and are carried as the field's - // default null value, which DefaultNullValueVirtualColumnProvider reads back. - if (BuiltInVirtualColumn.HOSTNAME.equals(column)) { - fieldSpec.setDefaultNullValue(NetUtils.getHostnameOrAddress()); - } else if (BuiltInVirtualColumn.SEGMENTNAME.equals(column)) { - fieldSpec.setDefaultNullValue(segmentName); - } - schema.addField(fieldSpec); } } + /// Creates the field spec a segment gets for one built-in virtual column: the shape from `definition`, the provider + /// class that produces its values, and for `$hostName` / `$segmentName` the constant value, carried as the field's + /// default null value, which `DefaultNullValueVirtualColumnProvider` reads back. Fresh per call: the spec is + /// mutable and the `$segmentName` value differs per segment, so it must not be shared across segments. + public static DimensionFieldSpec createBuiltInFieldSpec(BuiltInVirtualColumnDefinitions.Definition definition, + String segmentName) { + String column = definition.getName(); + DimensionFieldSpec fieldSpec = definition.createFieldSpec(); + fieldSpec.setVirtualColumnProvider(getProviderClass(column).getName()); + if (BuiltInVirtualColumn.HOSTNAME.equals(column)) { + fieldSpec.setDefaultNullValue(NetUtils.getHostnameOrAddress()); + } else if (BuiltInVirtualColumn.SEGMENTNAME.equals(column)) { + fieldSpec.setDefaultNullValue(segmentName); + } + return fieldSpec; + } + private static Class getProviderClass(String column) { Class providerClass = PROVIDER_CLASSES.get(column); Preconditions.checkState(providerClass != null, "No virtual column provider registered for built-in column: %s", diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/indexsegment/immutable/EmptyIndexSegmentTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/indexsegment/immutable/EmptyIndexSegmentTest.java index 14765f5f9145..f308cd6a6ca4 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/indexsegment/immutable/EmptyIndexSegmentTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/indexsegment/immutable/EmptyIndexSegmentTest.java @@ -18,15 +18,28 @@ */ package org.apache.pinot.segment.local.indexsegment.immutable; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.TreeMap; +import org.apache.pinot.segment.spi.ColumnMetadata; +import org.apache.pinot.segment.spi.index.metadata.EmptyColumnMetadata; import org.apache.pinot.segment.spi.index.metadata.SegmentMetadataImpl; import org.apache.pinot.segment.spi.store.SegmentDirectory; +import org.apache.pinot.spi.data.DimensionFieldSpec; +import org.apache.pinot.spi.data.FieldSpec; import org.testng.annotations.Test; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertThrows; +import static org.testng.Assert.assertTrue; public class EmptyIndexSegmentTest { @@ -80,6 +93,29 @@ public void testOnSegmentAddedNotifiesDirectoryAtMostOnce() verify(segmentDirectory, times(1)).onSegmentAdded(); } + /// Column listings are unmodifiable views of the column metadata map in key order (an empty segment registers no + /// virtual columns, so both listings agree) and never build the segment schema. + @Test + public void testColumnNamesComeFromColumnMetadata() { + SegmentMetadataImpl metadata = mock(SegmentMetadataImpl.class); + TreeMap columnMetadataMap = new TreeMap<>(); + for (String column : List.of("b", "a")) { + columnMetadataMap.put(column, + new EmptyColumnMetadata(new DimensionFieldSpec(column, FieldSpec.DataType.INT, true), null, null)); + } + when(metadata.getColumnMetadataMap()).thenReturn(columnMetadataMap); + EmptyIndexSegment segment = new EmptyIndexSegment(metadata); + + assertEquals(new ArrayList<>(segment.getColumnNames()), List.of("a", "b")); + assertEquals(new ArrayList<>(segment.getPhysicalColumnNames()), List.of("a", "b")); + assertEquals(segment.getPhysicalColumnNames(), Set.of("a", "b")); + assertTrue(segment.getPhysicalColumnNames().contains("a")); + assertFalse(segment.getPhysicalColumnNames().contains("c")); + assertThrows(UnsupportedOperationException.class, () -> segment.getColumnNames().remove("a")); + assertThrows(UnsupportedOperationException.class, () -> segment.getPhysicalColumnNames().add("c")); + verify(metadata, never()).getSchema(); + } + /// An empty segment loaded without a directory (e.g. the local File path) must treat the lifecycle callbacks as safe /// no-ops. @Test diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImplTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImplTest.java index 864615fdf77e..41a48241836f 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImplTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImplTest.java @@ -35,6 +35,7 @@ import javax.annotation.Nullable; import org.apache.pinot.segment.local.segment.index.map.ImmutableMapDataSource; import org.apache.pinot.segment.local.segment.index.openstruct.ImmutableOpenStructDataSource; +import org.apache.pinot.segment.local.segment.virtualcolumn.DocIdVirtualColumnProvider; import org.apache.pinot.segment.spi.ColumnMetadata; import org.apache.pinot.segment.spi.datasource.DataSource; import org.apache.pinot.segment.spi.index.StandardIndexes; @@ -47,7 +48,7 @@ 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.utils.CommonConstants.Segment.BuiltInVirtualColumn; import org.testng.annotations.Test; import static org.mockito.ArgumentMatchers.any; @@ -132,7 +133,7 @@ public void testEagerModeCreatesDataSourcesAtConstruction() { ColumnMetadataImpl a = columnMetadata(intColumn("a"), null); ColumnIndexContainer containerA = mock(ColumnIndexContainer.class); ImmutableSegmentImpl segment = - new ImmutableSegmentImpl(mock(SegmentDirectory.class), segmentMetadata(schema(a), a), Map.of("a", containerA), + new ImmutableSegmentImpl(mock(SegmentDirectory.class), segmentMetadata(a), Map.of("a", containerA), null); assertSame(segment.getDataSourceNullable("a").getIndexContainer(), containerA); @@ -146,10 +147,10 @@ public void testLazyModeCreatesNothingAtConstruction() ColumnMetadataImpl b = columnMetadata(intColumn("b"), null); ColumnMaterializer materializer = mock(ColumnMaterializer.class); SegmentDirectory segmentDirectory = mock(SegmentDirectory.class); - ImmutableSegmentImpl segment = lazySegment(segmentDirectory, schema(a, b), materializer, a, b); + ImmutableSegmentImpl segment = lazySegment(segmentDirectory, materializer, a, b); verifyNoInteractions(materializer); - // Column listings come from the metadata schema and never materialize anything + // Column listings are views of the column metadata and never materialize anything assertEquals(segment.getColumnNames(), Set.of("a", "b")); assertEquals(segment.getPhysicalColumnNames(), Set.of("a", "b")); // Neither does asking for a column the segment does not have @@ -174,7 +175,7 @@ public void testLazyModeMaterializesEachColumnOnceUnderConcurrentAccess() Thread.sleep(50); return containerA; }); - ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), schema(a), materializer, a); + ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), materializer, a); int numCallers = 16; ExecutorService executor = Executors.newFixedThreadPool(numCallers); @@ -214,7 +215,7 @@ public void testDestroyClosesOnlyMaterializedContainersAndRefusesLaterMaterializ when(materializer.createIndexContainer(a)).thenReturn(containerA); when(materializer.createIndexContainer(b)).thenReturn(containerB); SegmentDirectory segmentDirectory = mock(SegmentDirectory.class); - ImmutableSegmentImpl segment = lazySegment(segmentDirectory, schema(a, b), materializer, a, b); + ImmutableSegmentImpl segment = lazySegment(segmentDirectory, materializer, a, b); DataSource dataSourceA = segment.getDataSourceNullable("a"); assertNotNull(dataSourceA); @@ -242,7 +243,7 @@ public void testGetIndexMaterializesAndSharesTheContainerWithTheDataSource() { doReturn(forwardIndex).when(containerA).getIndex(StandardIndexes.forward()); ColumnMaterializer materializer = mock(ColumnMaterializer.class); when(materializer.createIndexContainer(a)).thenReturn(containerA); - ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), schema(a), materializer, a); + ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), materializer, a); assertSame(segment.getForwardIndex("a"), forwardIndex); verify(materializer, times(1)).createIndexContainer(a); @@ -274,9 +275,8 @@ public void testLazyModeGroupsOpenStructChildrenUnderTheirParent() ColumnMaterializer materializer = mock(ColumnMaterializer.class); when(materializer.createIndexContainer(views)).thenReturn(viewsContainer); when(materializer.createIndexContainer(sparse)).thenReturn(sparseContainer); - // Only the parent and the regular column are in the schema; the children live in the segment metadata alone - ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), schema(metrics, dim.getFieldSpec()), - materializer, parent, views, sparse, dim); + // The children are grouped under the parent whose column metadata declares it complex + ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), materializer, parent, views, sparse, dim); DataSource parentDataSource = segment.getDataSourceNullable("metrics"); assertTrue(parentDataSource instanceof ImmutableOpenStructDataSource); @@ -311,7 +311,7 @@ public void testLazyModeMapColumnYieldsMapDataSource() { doReturn(mock(ForwardIndexReader.class)).when(container).getIndex(StandardIndexes.forward()); ColumnMaterializer materializer = mock(ColumnMaterializer.class); when(materializer.createIndexContainer(m)).thenReturn(container); - ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), schema(mapSpec), materializer, m); + ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), materializer, m); DataSource dataSource = segment.getDataSourceNullable("m"); assertTrue(dataSource instanceof ImmutableMapDataSource); @@ -326,7 +326,7 @@ public void testLazyModeFailedMaterializationLeavesNoMappingAndRetrySucceeds() ColumnMaterializer materializer = mock(ColumnMaterializer.class); when(materializer.createIndexContainer(a)).thenThrow(new UncheckedIOException(new IOException("boom"))) .thenReturn(containerA); - ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), schema(a), materializer, a); + ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), materializer, a); assertThrows(UncheckedIOException.class, () -> segment.getDataSourceNullable("a")); DataSource dataSource = segment.getDataSourceNullable("a"); @@ -349,7 +349,7 @@ public void testLazyModeMaterializedContainersGetDataSourcesAtConstruction() ColumnMaterializer materializer = mock(ColumnMaterializer.class); ConcurrentMap materialized = new ConcurrentHashMap<>(Map.of("a", containerA)); ImmutableSegmentImpl segment = - new ImmutableSegmentImpl(mock(SegmentDirectory.class), segmentMetadata(schema(a, b), a, b), materializer, + new ImmutableSegmentImpl(mock(SegmentDirectory.class), segmentMetadata(a, b), materializer, materialized, null, null); verifyNoInteractions(materializer); @@ -360,17 +360,85 @@ public void testLazyModeMaterializedContainersGetDataSourcesAtConstruction() verify(containerA).close(); } - private static ImmutableSegmentImpl lazySegment(SegmentDirectory segmentDirectory, Schema schema, - ColumnMaterializer materializer, ColumnMetadata... columns) { - return new ImmutableSegmentImpl(segmentDirectory, segmentMetadata(schema, columns), materializer, + /// Column listings are views of the column metadata map: every column in key order, the physical ones without the + /// virtual columns (whose spec names a provider), unmodifiable, and never built from the segment schema, which + /// SegmentMetadataImpl derives on demand and a wide segment must not retain per column. The same in both modes. + @Test + public void testColumnNamesComeFromColumnMetadata() { + ColumnMetadataImpl b = columnMetadata(intColumn("b"), null); + ColumnMetadataImpl a = columnMetadata(intColumn("a"), null); + DimensionFieldSpec docIdSpec = new DimensionFieldSpec(BuiltInVirtualColumn.DOCID, FieldSpec.DataType.INT, true); + docIdSpec.setVirtualColumnProvider(DocIdVirtualColumnProvider.class.getName()); + ColumnMetadataImpl docId = columnMetadata(docIdSpec, null); + SegmentMetadataImpl segmentMetadata = segmentMetadata(b, a, docId); + Map containers = Map.of("a", mock(ColumnIndexContainer.class), "b", + mock(ColumnIndexContainer.class), BuiltInVirtualColumn.DOCID, mock(ColumnIndexContainer.class)); + ImmutableSegmentImpl eager = + new ImmutableSegmentImpl(mock(SegmentDirectory.class), segmentMetadata, containers, null); + ImmutableSegmentImpl lazy = + lazySegment(mock(SegmentDirectory.class), mock(ColumnMaterializer.class), b, a, docId); + + for (ImmutableSegmentImpl segment : List.of(eager, lazy)) { + assertEquals(new ArrayList<>(segment.getColumnNames()), List.of(BuiltInVirtualColumn.DOCID, "a", "b")); + assertEquals(new ArrayList<>(segment.getPhysicalColumnNames()), List.of("a", "b")); + assertEquals(segment.getColumnNames(), Set.of(BuiltInVirtualColumn.DOCID, "a", "b")); + assertEquals(segment.getPhysicalColumnNames(), Set.of("a", "b")); + assertEquals(segment.getPhysicalColumnNames().size(), 2); + assertTrue(segment.getPhysicalColumnNames().contains("a")); + assertFalse(segment.getPhysicalColumnNames().contains(BuiltInVirtualColumn.DOCID)); + assertFalse(segment.getPhysicalColumnNames().contains("c")); + assertThrows(UnsupportedOperationException.class, () -> segment.getColumnNames().remove("a")); + assertThrows(UnsupportedOperationException.class, () -> segment.getPhysicalColumnNames().remove("a")); + assertThrows(UnsupportedOperationException.class, () -> segment.getPhysicalColumnNames().add("c")); + } + verify(segmentMetadata, never()).getSchema(); + } + + /// The eager constructor finds the OPEN_STRUCT parent's ComplexFieldSpec in the column metadata rather than in the + /// segment schema, and groups the materialized children under it as before. + @Test + public void testEagerModeGroupsOpenStructChildrenUnderTheirParent() { + ComplexFieldSpec metrics = new ComplexFieldSpec("metrics", FieldSpec.DataType.OPEN_STRUCT, true, + Map.of("views", new DimensionFieldSpec("views", FieldSpec.DataType.LONG, true))); + String viewsColumn = OpenStructNaming.materializedColumnName("metrics", "views"); + String sparseColumn = OpenStructNaming.sparseColumnName("metrics"); + ColumnMetadataImpl parent = columnMetadata(metrics, null); + ColumnMetadataImpl views = columnMetadata(new DimensionFieldSpec(viewsColumn, FieldSpec.DataType.LONG, true), + "metrics"); + ColumnMetadataImpl sparse = columnMetadata(new DimensionFieldSpec(sparseColumn, FieldSpec.DataType.STRING, true), + "metrics"); + ColumnMetadataImpl dim = columnMetadata(intColumn("dim"), null); + ColumnIndexContainer viewsContainer = mock(ColumnIndexContainer.class); + SegmentMetadataImpl segmentMetadata = segmentMetadata(parent, views, sparse, dim); + ImmutableSegmentImpl segment = new ImmutableSegmentImpl(mock(SegmentDirectory.class), segmentMetadata, + Map.of(viewsColumn, viewsContainer, sparseColumn, mock(ColumnIndexContainer.class), "dim", + mock(ColumnIndexContainer.class)), null); + + DataSource parentDataSource = segment.getDataSourceNullable("metrics"); + assertTrue(parentDataSource instanceof ImmutableOpenStructDataSource); + ImmutableOpenStructDataSource openStruct = (ImmutableOpenStructDataSource) parentDataSource; + assertTrue(openStruct.isMaterialized("views")); + assertSame(openStruct.getDataSource("views").getIndexContainer(), viewsContainer); + // Children are reachable only through their parent + assertNull(segment.getDataSourceNullable(viewsColumn)); + assertNull(segment.getDataSourceNullable(sparseColumn)); + assertNotNull(segment.getDataSourceNullable("dim")); + assertEquals(segment.getPhysicalColumnNames(), Set.of("metrics", viewsColumn, sparseColumn, "dim")); + verify(segmentMetadata, never()).getSchema(); + } + + private static ImmutableSegmentImpl lazySegment(SegmentDirectory segmentDirectory, ColumnMaterializer materializer, + ColumnMetadata... columns) { + return new ImmutableSegmentImpl(segmentDirectory, segmentMetadata(columns), materializer, new ConcurrentHashMap<>(), null, null); } - private static SegmentMetadataImpl segmentMetadata(Schema schema, ColumnMetadata... columns) { + /// The segment must work from the column metadata map alone: getSchema() is left unstubbed (null) and is verified + /// never to be called where it matters. + private static SegmentMetadataImpl segmentMetadata(ColumnMetadata... columns) { SegmentMetadataImpl segmentMetadata = mock(SegmentMetadataImpl.class); when(segmentMetadata.getName()).thenReturn("seg"); when(segmentMetadata.getTotalDocs()).thenReturn(NUM_DOCS); - when(segmentMetadata.getSchema()).thenReturn(schema); TreeMap columnMetadataMap = new TreeMap<>(); for (ColumnMetadata column : columns) { columnMetadataMap.put(column.getColumnName(), column); @@ -379,22 +447,6 @@ private static SegmentMetadataImpl segmentMetadata(Schema schema, ColumnMetadata return segmentMetadata; } - private static Schema schema(FieldSpec... fieldSpecs) { - Schema schema = new Schema(); - for (FieldSpec fieldSpec : fieldSpecs) { - schema.addField(fieldSpec); - } - return schema; - } - - private static Schema schema(ColumnMetadata... columns) { - Schema schema = new Schema(); - for (ColumnMetadata column : columns) { - schema.addField(column.getFieldSpec()); - } - return schema; - } - private static FieldSpec intColumn(String name) { return new DimensionFieldSpec(name, FieldSpec.DataType.INT, true); } 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 ff841e06488d..c829c36c32a9 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 @@ -32,11 +32,14 @@ import java.util.concurrent.TimeUnit; import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.commons.io.FileUtils; +import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader; 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.local.segment.virtualcolumn.VirtualColumnProviderFactory; import org.apache.pinot.segment.spi.ColumnMetadata; +import org.apache.pinot.segment.spi.ImmutableSegment; import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig; import org.apache.pinot.segment.spi.creator.SegmentIndexCreationDriver; import org.apache.pinot.segment.spi.creator.SegmentVersion; @@ -54,7 +57,10 @@ 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.CommonConstants.Segment.BuiltInVirtualColumn; import org.apache.pinot.spi.utils.JsonUtils; +import org.apache.pinot.spi.utils.NetUtils; +import org.apache.pinot.spi.utils.ReadMode; import org.apache.pinot.spi.utils.builder.TableConfigBuilder; import org.apache.pinot.util.TestUtils; import org.testng.Assert; @@ -63,10 +69,12 @@ import org.testng.annotations.Test; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertNotSame; import static org.testng.Assert.assertNull; import static org.testng.Assert.assertSame; +import static org.testng.Assert.assertTrue; public class SegmentMetadataImplTest { @@ -285,6 +293,123 @@ public void testOpenStructChildSpecsSharedButParentIsNot() } } + /// A server retains the metadata of every loaded segment, and a Schema costs a TreeMap entry plus list slots per + /// column on top of the FieldSpecs the column metadata already holds, so the schema is derived on the first + /// getSchema() rather than at load. It equals the one that used to be built eagerly, is built exactly once, and + /// listing the columns or rendering the metadata JSON does not build it. + @Test + public void testSchemaDerivedLazilyFromColumnMetadata() + throws Exception { + long materializations = SegmentMetadataImpl.getNumSchemaMaterializations(); + SegmentMetadataImpl metadata = new SegmentMetadataImpl(_segmentDirectory); + assertFalse(metadata.isSchemaMaterialized()); + assertEquals(metadata.getAllColumns(), metadata.getColumnMetadataMap().keySet()); + assertEquals(metadata.toJson(null).get("columns").size(), metadata.getAllColumns().size()); + assertTrue(metadata.toJson(null).get("schemaName").isNull()); + assertFalse(metadata.isSchemaMaterialized()); + assertEquals(SegmentMetadataImpl.getNumSchemaMaterializations(), materializations); + + Schema eager = new Schema(); + for (ColumnMetadata columnMetadata : metadata.getColumnMetadataMap().values()) { + eager.addField(columnMetadata.getFieldSpec()); + } + Schema schema = metadata.getSchema(); + assertTrue(metadata.isSchemaMaterialized()); + assertEquals(SegmentMetadataImpl.getNumSchemaMaterializations(), materializations + 1); + assertEquals(schema, eager); + assertEquals(schema.getColumnNames(), metadata.getAllColumns()); + for (String column : metadata.getAllColumns()) { + assertSame(schema.getFieldSpecFor(column), metadata.getColumnMetadataFor(column).getFieldSpec(), column); + } + assertSame(metadata.getSchema(), schema); + assertEquals(SegmentMetadataImpl.getNumSchemaMaterializations(), materializations + 1); + } + + /// Loading a segment registers the built-in virtual columns in the column metadata, so the schema derived afterwards + /// includes them exactly as the schema the loader used to build eagerly did, while neither the load nor serving the + /// segment (column listings, data sources, the metadata JSON) builds any schema. + @Test + public void testSchemaIncludesBuiltInVirtualColumnsAfterLoad() + throws Exception { + long materializations = SegmentMetadataImpl.getNumSchemaMaterializations(); + ImmutableSegment segment = ImmutableSegmentLoader.load(_segmentDirectory, ReadMode.mmap); + try { + SegmentMetadataImpl metadata = (SegmentMetadataImpl) segment.getSegmentMetadata(); + assertFalse(metadata.isSchemaMaterialized(), "the load path must not build the segment schema"); + assertTrue(segment.getColumnNames().containsAll(BuiltInVirtualColumn.BUILT_IN_VIRTUAL_COLUMNS)); + assertTrue(metadata.getAllColumns().containsAll(BuiltInVirtualColumn.BUILT_IN_VIRTUAL_COLUMNS)); + assertFalse(segment.getPhysicalColumnNames().contains(BuiltInVirtualColumn.DOCID)); + assertEquals(segment.getPhysicalColumnNames().size(), + segment.getColumnNames().size() - BuiltInVirtualColumn.BUILT_IN_VIRTUAL_COLUMNS.size()); + for (String column : segment.getColumnNames()) { + assertNotNull(segment.getDataSource(column), column); + } + metadata.toJson(null); + assertFalse(metadata.isSchemaMaterialized(), "serving the segment must not build the segment schema"); + assertEquals(SegmentMetadataImpl.getNumSchemaMaterializations(), materializations); + + Schema legacy = new Schema(); + for (String column : segment.getPhysicalColumnNames()) { + legacy.addField(metadata.getColumnMetadataFor(column).getFieldSpec()); + } + VirtualColumnProviderFactory.addBuiltInVirtualColumnsToSegmentSchema(legacy, metadata.getName()); + Schema schema = metadata.getSchema(); + assertEquals(schema, legacy); + assertEquals(schema.getColumnNames(), metadata.getAllColumns()); + assertEquals(schema.getPhysicalColumnNames(), segment.getPhysicalColumnNames()); + for (String column : BuiltInVirtualColumn.BUILT_IN_VIRTUAL_COLUMNS) { + FieldSpec fieldSpec = schema.getFieldSpecFor(column); + assertTrue(fieldSpec.isVirtualColumn(), column); + assertSame(fieldSpec, metadata.getColumnMetadataFor(column).getFieldSpec(), column); + } + assertEquals(schema.getFieldSpecFor(BuiltInVirtualColumn.SEGMENTNAME).getDefaultNullValue(), + metadata.getName()); + assertEquals(schema.getFieldSpecFor(BuiltInVirtualColumn.HOSTNAME).getDefaultNullValue(), + NetUtils.getHostnameOrAddress()); + } finally { + segment.destroy(); + } + } + + /// removeColumn() drops the column from the column metadata and from any schema derived afterwards. + @Test + public void testRemoveColumnInvalidatesDerivedSchema() + throws Exception { + SegmentMetadataImpl metadata = new SegmentMetadataImpl(_segmentDirectory); + Schema before = metadata.getSchema(); + String column = metadata.getAllColumns().stream().filter(c -> !c.equals(metadata.getTimeColumn())).findFirst() + .orElseThrow(); + assertTrue(before.hasColumn(column)); + + metadata.removeColumn(column); + assertFalse(metadata.isSchemaMaterialized()); + assertFalse(metadata.getAllColumns().contains(column)); + assertNull(metadata.getColumnMetadataFor(column)); + Schema after = metadata.getSchema(); + assertNotSame(after, before); + assertFalse(after.hasColumn(column)); + assertEquals(after.size(), before.size() - 1); + assertEquals(after.getColumnNames(), metadata.getAllColumns()); + } + + /// A CONSUMING segment is constructed with its schema, which is handed back as is (and named in the JSON) rather + /// than derived: it has no column metadata to derive from. + @Test + public void testExplicitSchemaIsReturnedAsIs() { + long materializations = SegmentMetadataImpl.getNumSchemaMaterializations(); + Schema schema = new Schema.SchemaBuilder().setSchemaName("consuming") + .addSingleValueDimension("dim", FieldSpec.DataType.STRING) + .addMetric("metric", FieldSpec.DataType.LONG) + .build(); + SegmentMetadataImpl metadata = + new SegmentMetadataImpl("testTable", "testTable__0__0__20240101T0000Z", schema, 123L); + assertTrue(metadata.isSchemaMaterialized()); + assertSame(metadata.getSchema(), schema); + assertEquals(metadata.getAllColumns(), schema.getColumnNames()); + assertEquals(metadata.toJson(null).get("schemaName").asText(), "consuming"); + assertEquals(SegmentMetadataImpl.getNumSchemaMaterializations(), materializations); + } + private static File buildOpenStructSegment(String parent) throws Exception { Map children = new HashMap<>(); diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/LoaderTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/LoaderTest.java index a1832ae461e2..a1bf466bdbc6 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/LoaderTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/LoaderTest.java @@ -230,7 +230,13 @@ private void testBuiltInVirtualColumns(IndexSegment indexSegment) { assertTrue(indexSegment.getColumnNames().containsAll(BuiltInVirtualColumn.BUILT_IN_VIRTUAL_COLUMNS)); for (String column : BuiltInVirtualColumn.BUILT_IN_VIRTUAL_COLUMNS) { assertNotNull(indexSegment.getDataSource(column), "Missing data source for virtual column: " + column); + assertFalse(indexSegment.getPhysicalColumnNames().contains(column), column + " is not a physical column"); } + // The virtual columns are registered in the column metadata, from which the segment schema is derived on demand. + // A load (v1 or v3, with the preprocess check) must leave that schema unbuilt: a stray getSchema() on the load + // path would re-inflate the per-column schema footprint of every segment a server loads. + assertFalse(((SegmentMetadataImpl) indexSegment.getSegmentMetadata()).isSchemaMaterialized(), + "the load path must not build the segment schema"); // Segment metadata that this segment carries is exposed as a real value, and is not marked null SegmentMetadata segmentMetadata = indexSegment.getSegmentMetadata(); diff --git a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java index 8559134b5686..c1c3ceb42997 100644 --- a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java +++ b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java @@ -70,6 +70,10 @@ public interface SegmentMetadata { /// segment (of this table or any other) whose column parses to an equal spec, and must be treated as immutable: never /// call a setter on one; copy it (e.g. through a JSON round-trip) before mutating. Removing a column from this schema /// does not affect other segments. + /// + /// An implementation may derive the schema on demand rather than hold it per segment, so load- and query-path code + /// should read column names through [#getAllColumns()] and field specs through [#getColumnMetadataFor(String)] + /// instead of building a schema for every segment it touches. Schema getSchema(); int getTotalDocs(); 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 d47fa0d56b04..6fc61b5233e6 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 @@ -71,10 +71,26 @@ /// one instance per distinct spec instead of retaining its own. Segment-derived [FieldSpec]s must therefore be treated /// as immutable: a setter call on one would bleed into every other segment and table that shares it, and would /// corrupt the interner's hash bucket (nothing ever mutated one; copy via a JSON round-trip before mutating). +/// +/// The object layout is kept at 72 bytes for an ordinary column for the same reason: the six booleans and the +/// forward-index encoding are packed into one [#_flags] byte, and the refs only a partitioned column or an OPEN_STRUCT +/// parent/child carries (partition function and partitions, parent column, sparse keys) live in a lazily allocated +/// [Extras] holder that stays `null` for every other column. The compression stats stay a direct ref because the +/// segment creator writes them for every raw column. None of this is visible through the public getters, so the +/// `/tables/{table}/segments/{segment}/metadata` payload (bean-serialized from the getters) is unchanged. @SuppressWarnings({"rawtypes", "unchecked"}) public class ColumnMetadataImpl implements ColumnMetadata { private static final long SIZE_MASK = 0xffffffffffffL; + // Bits of _flags + private static final byte HAS_DICTIONARY = 1; + private static final byte DICTIONARY_ENCODED_FORWARD_INDEX = 1 << 1; + private static final byte SORTED = 1 << 2; + private static final byte NON_NULL = 1 << 3; + private static final byte MIN_MAX_VALUE_INVALID = 1 << 4; + private static final byte ASCII = 1 << 5; + private static final byte AUTO_GENERATED = 1 << 6; + /// Canonical instances of the [FieldSpec]s parsed from `metadata.properties`, keyed by [FieldSpec#equals] / /// [FieldSpec#hashCode] (name, data type, single-value, default null value, max length, date-time format and /// granularity, ...), so schema evolution yields a distinct canonical instance per version of a column. The specs @@ -85,27 +101,21 @@ public class ColumnMetadataImpl implements ColumnMetadata { private final FieldSpec _fieldSpec; private final int _totalDocs; private final int _cardinality; - private final boolean _hasDictionary; - private final EncodingType _forwardIndexEncoding; - private final boolean _sorted; - private final boolean _nonNull; + @Nullable private final Comparable _minValue; + @Nullable private final Comparable _maxValue; - private final boolean _minMaxValueInvalid; private final int _lengthOfShortestElement; private final int _lengthOfLongestElement; - private final boolean _isAscii; private final int _totalNumberOfEntries; private final int _maxNumberOfMultiValues; private final int _maxRowLengthInBytes; private final int _bitsPerElement; - private final PartitionFunction _partitionFunction; - private final Set _partitions; - private final boolean _autoGenerated; - @Nullable - private final String _parentColumn; + /// hasDictionary, forward-index encoding, sorted, nonNull, minMaxValueInvalid, ascii and autoGenerated, see the + /// bit constants above. + private final byte _flags; @Nullable - private final List _sparseKeys; + private final Extras _extras; @Nullable private final CompressionMetadata _compressionMetadata; @@ -122,39 +132,30 @@ public class ColumnMetadataImpl implements ColumnMetadata { @Nullable private long[] _indexTypeSizes; - private ColumnMetadataImpl(FieldSpec fieldSpec, int totalDocs, int cardinality, boolean hasDictionary, - @Nullable EncodingType forwardIndexEncoding, boolean sorted, boolean nonNull, @Nullable Comparable minValue, - @Nullable Comparable maxValue, - boolean minMaxValueInvalid, int lengthOfShortestElement, int lengthOfLongestElement, boolean isAscii, - int totalNumberOfEntries, int maxNumberOfMultiValues, int maxRowLengthInBytes, int bitsPerElement, - @Nullable PartitionFunction partitionFunction, @Nullable Set partitions, boolean autoGenerated, - @Nullable String parentColumn, @Nullable List sparseKeys, - @Nullable CompressionMetadata compressionMetadata) { + private ColumnMetadataImpl(FieldSpec fieldSpec, int totalDocs, int cardinality, @Nullable Comparable minValue, + @Nullable Comparable maxValue, int lengthOfShortestElement, int lengthOfLongestElement, + int totalNumberOfEntries, int maxNumberOfMultiValues, int maxRowLengthInBytes, int bitsPerElement, byte flags, + @Nullable Extras extras, @Nullable CompressionMetadata compressionMetadata) { _fieldSpec = fieldSpec; _totalDocs = totalDocs; _cardinality = cardinality; - _hasDictionary = hasDictionary; - _forwardIndexEncoding = forwardIndexEncoding; - _sorted = sorted; - _nonNull = nonNull; _minValue = minValue; _maxValue = maxValue; - _minMaxValueInvalid = minMaxValueInvalid; _lengthOfShortestElement = lengthOfShortestElement; _lengthOfLongestElement = lengthOfLongestElement; - _isAscii = isAscii; - _bitsPerElement = bitsPerElement; _totalNumberOfEntries = totalNumberOfEntries; _maxNumberOfMultiValues = maxNumberOfMultiValues; _maxRowLengthInBytes = maxRowLengthInBytes; - _partitionFunction = partitionFunction; - _partitions = partitions; - _autoGenerated = autoGenerated; - _parentColumn = parentColumn; - _sparseKeys = sparseKeys; + _bitsPerElement = bitsPerElement; + _flags = flags; + _extras = extras; _compressionMetadata = compressionMetadata; } + private boolean hasFlag(byte flag) { + return (_flags & flag) != 0; + } + @Override public FieldSpec getFieldSpec() { return _fieldSpec; @@ -172,22 +173,22 @@ public int getCardinality() { @Override public boolean hasDictionary() { - return _hasDictionary; + return hasFlag(HAS_DICTIONARY); } @Override public EncodingType getForwardIndexEncoding() { - return _forwardIndexEncoding; + return hasFlag(DICTIONARY_ENCODED_FORWARD_INDEX) ? EncodingType.DICTIONARY : EncodingType.RAW; } @Override public boolean isSorted() { - return _sorted; + return hasFlag(SORTED); } @Override public boolean isNonNull() { - return _nonNull; + return hasFlag(NON_NULL); } @Nullable @@ -204,7 +205,7 @@ public Comparable getMaxValue() { @Override public boolean isMinMaxValueInvalid() { - return _minMaxValueInvalid; + return hasFlag(MIN_MAX_VALUE_INVALID); } @Override @@ -219,7 +220,7 @@ public int getLengthOfLongestElement() { @Override public boolean isAscii() { - return _isAscii; + return hasFlag(ASCII); } @Override @@ -245,36 +246,36 @@ public int getMaxRowLengthInBytes() { @Nullable @Override public PartitionFunction getPartitionFunction() { - return _partitionFunction; + return _extras != null ? _extras._partitionFunction : null; } @Nullable @Override public Set getPartitions() { - return _partitions; + return _extras != null ? _extras._partitions : null; } @Override public boolean isAutoGenerated() { - return _autoGenerated; + return hasFlag(AUTO_GENERATED); } /// Returns `true` if this column is a materialized column produced from an OPEN_STRUCT parent column. public boolean isMaterializedChild() { - return _parentColumn != null; + return getParentColumn() != null; } /// Returns the name of the parent OPEN_STRUCT column, or `null` if this is not a materialized column. @Nullable public String getParentColumn() { - return _parentColumn; + return _extras != null ? _extras._parentColumn : null; } /// Names of the keys in this OPEN_STRUCT column's sparse blob, or null when unknown /// (segment predates the manifest). Only set on OPEN_STRUCT parent columns. @Nullable public List getSparseKeys() { - return _sparseKeys; + return _extras != null ? _extras._sparseKeys : null; } @Override @@ -355,62 +356,53 @@ public boolean equals(Object o) { ColumnMetadataImpl that = (ColumnMetadataImpl) o; return _totalDocs == that._totalDocs && _cardinality == that._cardinality - && _hasDictionary == that._hasDictionary - && _forwardIndexEncoding == that._forwardIndexEncoding - && _sorted == that._sorted && _nonNull == that._nonNull - && _minMaxValueInvalid == that._minMaxValueInvalid + && _flags == that._flags && _lengthOfShortestElement == that._lengthOfShortestElement && _lengthOfLongestElement == that._lengthOfLongestElement - && _isAscii == that._isAscii && _totalNumberOfEntries == that._totalNumberOfEntries && _maxNumberOfMultiValues == that._maxNumberOfMultiValues && _maxRowLengthInBytes == that._maxRowLengthInBytes && _bitsPerElement == that._bitsPerElement - && _autoGenerated == that._autoGenerated && Objects.equals(_fieldSpec, that._fieldSpec) && Objects.equals(_minValue, that._minValue) && Objects.equals(_maxValue, that._maxValue) - && Objects.equals(_partitionFunction, that._partitionFunction) - && Objects.equals(_partitions, that._partitions) - && Objects.equals(_parentColumn, that._parentColumn) - && Objects.equals(_sparseKeys, that._sparseKeys) + && Objects.equals(_extras, that._extras) && Objects.equals(_compressionMetadata, that._compressionMetadata) && Arrays.equals(_indexTypeSizes, that._indexTypeSizes); } @Override public int hashCode() { - return Objects.hash(_fieldSpec, _totalDocs, _cardinality, _hasDictionary, _forwardIndexEncoding, _sorted, _nonNull, - _minValue, _maxValue, _minMaxValueInvalid, _lengthOfShortestElement, _lengthOfLongestElement, _isAscii, - _totalNumberOfEntries, _maxNumberOfMultiValues, _maxRowLengthInBytes, _bitsPerElement, _partitionFunction, - _partitions, _autoGenerated, _parentColumn, _sparseKeys, _compressionMetadata, - Arrays.hashCode(_indexTypeSizes)); + return Objects.hash(_fieldSpec, _totalDocs, _cardinality, _flags, _minValue, _maxValue, _lengthOfShortestElement, + _lengthOfLongestElement, _totalNumberOfEntries, _maxNumberOfMultiValues, _maxRowLengthInBytes, _bitsPerElement, + _extras, _compressionMetadata, Arrays.hashCode(_indexTypeSizes)); } + // Keeps the pre-packing field names and order, which tests and log consumers match on @Override public String toString() { return "ColumnMetadataImpl{" + "_fieldSpec=" + _fieldSpec + ", _totalDocs=" + _totalDocs + ", _cardinality=" + _cardinality - + ", _hasDictionary=" + _hasDictionary - + ", _forwardIndexEncoding=" + _forwardIndexEncoding - + ", _sorted=" + _sorted + ", _nonNull=" + _nonNull + + ", _hasDictionary=" + hasDictionary() + + ", _forwardIndexEncoding=" + getForwardIndexEncoding() + + ", _sorted=" + isSorted() + ", _nonNull=" + isNonNull() + ", _minValue=" + _minValue + ", _maxValue=" + _maxValue - + ", _minMaxValueInvalid=" + _minMaxValueInvalid + + ", _minMaxValueInvalid=" + isMinMaxValueInvalid() + ", _lengthOfShortestElement=" + _lengthOfShortestElement + ", _lengthOfLongestElement=" + _lengthOfLongestElement - + ", _isAscii=" + _isAscii + + ", _isAscii=" + isAscii() + ", _totalNumberOfEntries=" + _totalNumberOfEntries + ", _maxNumberOfMultiValues=" + _maxNumberOfMultiValues + ", _maxRowLengthInBytes=" + _maxRowLengthInBytes + ", _bitsPerElement=" + _bitsPerElement - + ", _partitionFunction=" + _partitionFunction - + ", _partitions=" + _partitions - + ", _autoGenerated=" + _autoGenerated - + ", _parentColumn=" + _parentColumn - + ", _sparseKeys=" + _sparseKeys + + ", _partitionFunction=" + getPartitionFunction() + + ", _partitions=" + getPartitions() + + ", _autoGenerated=" + isAutoGenerated() + + ", _parentColumn=" + getParentColumn() + + ", _sparseKeys=" + getSparseKeys() + ", _compressionMetadata=" + _compressionMetadata + ", _indexTypeSizes=" + Arrays.toString(_indexTypeSizes) + '}'; @@ -675,6 +667,55 @@ public static Builder builder() { return new Builder(); } + /// The refs that only a partitioned column (partition function and partitions) or an OPEN_STRUCT parent/child + /// (sparse keys / parent column) carries. Ordinary columns hold no holder at all, so they never pay for the four + /// slots; a column that has any of them pays one extra object. + private static final class Extras { + @Nullable + private final PartitionFunction _partitionFunction; + @Nullable + private final Set _partitions; + @Nullable + private final String _parentColumn; + @Nullable + private final List _sparseKeys; + + private Extras(@Nullable PartitionFunction partitionFunction, @Nullable Set partitions, + @Nullable String parentColumn, @Nullable List sparseKeys) { + _partitionFunction = partitionFunction; + _partitions = partitions; + _parentColumn = parentColumn; + _sparseKeys = sparseKeys; + } + + @Nullable + private static Extras create(@Nullable PartitionFunction partitionFunction, @Nullable Set partitions, + @Nullable String parentColumn, @Nullable List sparseKeys) { + return partitionFunction == null && partitions == null && parentColumn == null && sparseKeys == null ? null + : new Extras(partitionFunction, partitions, parentColumn, sparseKeys); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + Extras that = (Extras) o; + return Objects.equals(_partitionFunction, that._partitionFunction) + && Objects.equals(_partitions, that._partitions) + && Objects.equals(_parentColumn, that._parentColumn) + && Objects.equals(_sparseKeys, that._sparseKeys); + } + + @Override + public int hashCode() { + return Objects.hash(_partitionFunction, _partitions, _parentColumn, _sparseKeys); + } + } + private static final class CompressionMetadata { private final long _uncompressedValueSizeInBytes; @Nullable @@ -927,10 +968,32 @@ public ColumnMetadataImpl build() { _bitsPerElement = UNAVAILABLE; } - return new ColumnMetadataImpl(_fieldSpec, _totalDocs, _cardinality, _hasDictionary, _forwardIndexEncoding, - _sorted, _nonNull, _minValue, _maxValue, _minMaxValueInvalid, _lengthOfShortestElement, - _lengthOfLongestElement, _isAscii, _totalNumberOfEntries, _maxNumberOfMultiValues, _maxRowLengthInBytes, - _bitsPerElement, _partitionFunction, _partitions, _autoGenerated, _parentColumn, _sparseKeys, + byte flags = 0; + if (_hasDictionary) { + flags |= HAS_DICTIONARY; + } + if (_forwardIndexEncoding == EncodingType.DICTIONARY) { + flags |= DICTIONARY_ENCODED_FORWARD_INDEX; + } + if (_sorted) { + flags |= SORTED; + } + if (_nonNull) { + flags |= NON_NULL; + } + if (_minMaxValueInvalid) { + flags |= MIN_MAX_VALUE_INVALID; + } + if (_isAscii) { + flags |= ASCII; + } + if (_autoGenerated) { + flags |= AUTO_GENERATED; + } + return new ColumnMetadataImpl(_fieldSpec, _totalDocs, _cardinality, _minValue, _maxValue, + _lengthOfShortestElement, _lengthOfLongestElement, _totalNumberOfEntries, _maxNumberOfMultiValues, + _maxRowLengthInBytes, _bitsPerElement, flags, + Extras.create(_partitionFunction, _partitions, _parentColumn, _sparseKeys), CompressionMetadata.create(_uncompressedValueSizeInBytes, _forwardIndexChunkCompressionType, _dictionaryUncompressedValueSizeInBytes)); } 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 b57434107b45..5bdc00574109 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 @@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import java.io.DataInputStream; import java.io.File; @@ -36,10 +37,12 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.NavigableSet; import java.util.Set; import java.util.TimeZone; import java.util.TreeMap; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; import javax.annotation.Nullable; import org.apache.commons.configuration2.Configuration; import org.apache.commons.configuration2.PropertiesConfiguration; @@ -70,12 +73,34 @@ import org.slf4j.LoggerFactory; +/// Segment metadata parsed from `metadata.properties` (plus `creation.meta` and the v3 `index_map`), or built for a +/// CONSUMING segment from an explicit [Schema]. +/// +/// The segment [Schema] of a metadata-backed instance is derived from [#getColumnMetadataMap()] on the first +/// [#getSchema()] call and cached; it is not built at load. A server retains one instance per loaded segment for the +/// segment's lifetime, and a Schema costs a `TreeMap` entry plus list slots per column on top of the column metadata +/// that already holds every [org.apache.pinot.spi.data.FieldSpec], so building it eagerly doubled the per-column +/// metadata footprint of a wide segment that is never asked for its schema. Everything on the load and query paths +/// reads the column metadata map (or [#getAllColumns()], a view of its keys) instead. Once the loader has registered +/// the built-in virtual columns in the map, the derived schema includes them, exactly as the eagerly built one did. +/// [#removeColumn(String)] drops the cached schema so it is rebuilt without the column. The explicit-schema +/// constructor keeps the caller's Schema as is. +/// +/// Thread-safe for the schema cache (double-checked on a volatile, so one instance per metadata); the rest is +/// populated at load before the metadata is published. public class SegmentMetadataImpl implements SegmentMetadata { private static final Logger LOGGER = LoggerFactory.getLogger(SegmentMetadataImpl.class); + /// Number of derived schemas built so far, JVM-wide, so a test can assert that a load or a query left every + /// segment's schema unbuilt. + private static final AtomicLong NUM_SCHEMA_MATERIALIZATIONS = new AtomicLong(); + private final File _indexDir; private final TreeMap _columnMetadataMap; - private final Schema _schema; + /// The explicit schema of a CONSUMING segment, or the lazily derived schema of a metadata-backed segment (null + /// until [#getSchema()] builds it, and again after [#removeColumn(String)]). + @Nullable + private volatile Schema _schema; private String _segmentName; private int _totalDocs; private SegmentVersion _segmentVersion; @@ -109,7 +134,6 @@ public SegmentMetadataImpl(InputStream metadataPropertiesInputStream, InputStrea throws IOException, ConfigurationException { _indexDir = null; _columnMetadataMap = new TreeMap<>(); - _schema = new Schema(); PropertiesConfiguration segmentMetadataPropertiesConfiguration = CommonsConfigurationUtils.fromInputStream(metadataPropertiesInputStream); @@ -128,7 +152,6 @@ public SegmentMetadataImpl(File indexDir) throws IOException, ConfigurationException { _indexDir = indexDir; _columnMetadataMap = new TreeMap<>(); - _schema = new Schema(); PropertiesConfiguration segmentMetadataPropertiesConfiguration = SegmentMetadataUtils.getPropertiesConfiguration(indexDir); @@ -231,14 +254,13 @@ private void init(PropertiesConfiguration segmentMetadata) addPhysicalColumns(segmentMetadata.getList(Segment.DATETIME_COLUMNS), physicalColumns); addPhysicalColumns(segmentMetadata.getList(Segment.COMPLEX_COLUMNS), physicalColumns); - // Build column metadata map and schema. Empty segments use a stripped-down [EmptyColumnMetadata] since the - // shape stats (cardinality, element lengths, etc.) are meaningless when there are no rows. + // Build the column metadata map (the schema is derived from it on demand, see getSchema()). Empty segments use a + // stripped-down [EmptyColumnMetadata] since the shape stats (cardinality, element lengths, etc.) are meaningless + // when there are no rows. if (_totalDocs > 0) { for (String column : physicalColumns) { - ColumnMetadata columnMetadata = - ColumnMetadataImpl.fromPropertiesConfiguration(segmentMetadata, _totalDocs, column); - _columnMetadataMap.put(column, columnMetadata); - _schema.addField(columnMetadata.getFieldSpec()); + _columnMetadataMap.put(column, + ColumnMetadataImpl.fromPropertiesConfiguration(segmentMetadata, _totalDocs, column)); } // Load index metadata @@ -267,9 +289,7 @@ private void init(PropertiesConfiguration segmentMetadata) } } else { for (String column : physicalColumns) { - ColumnMetadata columnMetadata = EmptyColumnMetadata.fromPropertiesConfiguration(segmentMetadata, column); - _columnMetadataMap.put(column, columnMetadata); - _schema.addField(columnMetadata.getFieldSpec()); + _columnMetadataMap.put(column, EmptyColumnMetadata.fromPropertiesConfiguration(segmentMetadata, column)); } } @@ -386,9 +406,56 @@ public SegmentVersion getVersion() { return _segmentVersion; } + /// {@inheritDoc} + /// + /// For a metadata-backed segment the schema is built from the column metadata map on the first call (one + /// `FieldSpec` per column, the built-in virtual columns included once the loader has registered them) and cached + /// until [#removeColumn(String)]. Nothing on the load or query path should call this: a caller there re-inflates + /// the per-column schema footprint for every segment it touches. Column names are available through + /// [#getAllColumns()] and field specs through [#getColumnMetadataFor(String)]. @Override public Schema getSchema() { - return _schema; + Schema schema = _schema; + if (schema == null) { + synchronized (this) { + schema = _schema; + if (schema == null) { + schema = buildSchema(); + _schema = schema; + } + } + } + return schema; + } + + private Schema buildSchema() { + NUM_SCHEMA_MATERIALIZATIONS.incrementAndGet(); + Schema schema = new Schema(); + for (ColumnMetadata columnMetadata : _columnMetadataMap.values()) { + schema.addField(columnMetadata.getFieldSpec()); + } + return schema; + } + + /// Whether [#getSchema()] has been called (and its schema cached) since construction or the last + /// [#removeColumn(String)]. Always `true` for a CONSUMING segment, which is constructed with its schema. + @VisibleForTesting + public boolean isSchemaMaterialized() { + return _schema != null; + } + + /// Number of schemas derived from column metadata so far in this JVM. A load or query path that leaves this + /// unchanged did not build any segment's schema. + @VisibleForTesting + public static long getNumSchemaMaterializations() { + return NUM_SCHEMA_MATERIALIZATIONS.get(); + } + + /// The keys of the column metadata map, i.e. the same names as `getSchema().getColumnNames()` without building the + /// schema. Falls back to the explicit schema of a CONSUMING segment, which has no column metadata map. + @Override + public NavigableSet getAllColumns() { + return _columnMetadataMap != null ? _columnMetadataMap.navigableKeySet() : getSchema().getColumnNames(); } @Override @@ -492,14 +559,17 @@ public TreeMap getColumnMetadataMap() { public void removeColumn(String column) { Preconditions.checkState(!column.equals(_timeColumn), "Cannot remove time column: %s", _timeColumn); _columnMetadataMap.remove(column); - _schema.removeField(column); + // Drop the derived schema, if one was built, so the next getSchema() rebuilds it without the column + _schema = null; } @Override public JsonNode toJson(@Nullable Set columnFilter) { ObjectNode segmentMetadata = JsonUtils.newObjectNode(); segmentMetadata.put("segmentName", _segmentName); - segmentMetadata.put("schemaName", _schema != null ? _schema.getSchemaName() : null); + // Only an explicit (CONSUMING segment) schema carries a name; a derived one never does, so it is not built here + Schema schema = _schema; + segmentMetadata.put("schemaName", schema != null ? schema.getSchemaName() : null); segmentMetadata.put("crc", _crc); if (_dataCrc != Long.MIN_VALUE) { segmentMetadata.put("dataCrc", _dataCrc); 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 6e6aea4e7b93..437918a67880 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 @@ -24,6 +24,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeSet; import java.util.UUID; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; @@ -32,6 +33,8 @@ import org.apache.pinot.segment.spi.V1Constants.MetadataKeys.Column; import org.apache.pinot.segment.spi.V1Constants.MetadataKeys.Segment; import org.apache.pinot.segment.spi.compression.ChunkCompressionType; +import org.apache.pinot.segment.spi.partition.PartitionFunction; +import org.apache.pinot.segment.spi.partition.PartitionIdNormalizer; import org.apache.pinot.spi.config.table.FieldConfig.EncodingType; import org.apache.pinot.spi.data.ComplexFieldSpec; import org.apache.pinot.spi.data.DateTimeFieldSpec; @@ -225,6 +228,223 @@ public void compressionStatsDoNotExpandExistingColumnMetadataJson() { assertFalse(json.has("dictionaryUncompressedValueSizeInBytes")); } + /// `/tables/{table}/segments/{segment}/metadata` bean-serializes the column metadata, so its payload is exactly the + /// set of public getters. The in-memory layout (the flags byte, the lazily allocated holder for the rarely populated + /// refs) is private and must never leak into it, and a getter added, renamed or annotated shows up here. + @Test + public void jsonPropertySetIsUnchanged() { + Set expected = new TreeSet<>(List.of("ascii", "autoGenerated", "bitsPerElement", "cardinality", + "columnName", "dataType", "fieldSpec", "fieldType", "fixedLength", "forwardIndexEncoding", "hasDictionary", + "indexSizeMap", "lengthOfLongestElement", "lengthOfShortestElement", "materializedChild", + "maxNumberOfMultiValues", "maxRowLengthInBytes", "maxValue", "minMaxValueInvalid", "minValue", "nonNull", + "parentColumn", "partitionFunction", "partitions", "singleValue", "sorted", "sparseKeys", "storedType", + "totalDocs", "totalNumberOfEntries")); + for (ColumnMetadataImpl metadata : List.of(fullyPopulated().build(), + ColumnMetadataImpl.fromPropertiesConfiguration(baseConfig("col"), 1, "col"))) { + Set keys = new TreeSet<>(); + JsonUtils.objectToJsonNode(metadata).fieldNames().forEachRemaining(keys::add); + assertEquals(keys, expected); + } + } + + /// Partition info, the OPEN_STRUCT parent link and the sparse-key manifest are rare, so the metadata keeps them in + /// a lazily allocated holder; this drives the populated path through every accessor and value-object method. + @Test + public void rarelyPopulatedFieldsRoundTrip() { + ColumnMetadataImpl metadata = fullyPopulated().build(); + assertSame(metadata.getPartitionFunction(), PARTITION_FUNCTION); + assertEquals(metadata.getPartitions(), Set.of(1, 3)); + assertEquals(metadata.getParentColumn(), "metrics"); + assertTrue(metadata.isMaterializedChild()); + assertEquals(metadata.getSparseKeys(), List.of("region", "latencyMs")); + assertEquals(metadata.getRawForwardIndexUncompressedValueSizeInBytes(), 4096L); + assertEquals(metadata.getRawForwardIndexChunkCompressionType(), ChunkCompressionType.LZ4); + assertEquals(metadata.getDictionaryEncodedUncompressedValueSizeInBytes(), 2048L); + + assertEquals(fullyPopulated().build(), metadata); + assertEquals(fullyPopulated().build().hashCode(), metadata.hashCode()); + assertNotEquals(fullyPopulated().setPartitionFunction(new TestPartitionFunction(8)).build(), metadata); + assertNotEquals(fullyPopulated().setPartitions(Set.of(2)).build(), metadata); + assertNotEquals(fullyPopulated().setParentColumn("other").build(), metadata); + assertNotEquals(fullyPopulated().setSparseKeys(List.of("region")).build(), metadata); + assertNotEquals(fullyPopulated().setDictionaryEncodedUncompressedValueSizeInBytes(1).build(), metadata); + + String string = metadata.toString(); + assertTrue(string.contains("_partitionFunction=TestPartitionFunction{4}"), string); + assertTrue(string.contains("_partitions=[1, 3]"), string); + assertTrue(string.contains("_parentColumn=metrics"), string); + assertTrue(string.contains("_sparseKeys=[region, latencyMs]"), string); + assertTrue(string.contains("_compressionMetadata=CompressionMetadata{_uncompressedValueSizeInBytes=4096"), string); + + JsonNode json = JsonUtils.objectToJsonNode(metadata); + assertEquals(json.get("partitionFunction").get("numPartitions").asInt(), 4); + assertEquals(json.get("partitions").size(), 2); + assertEquals(json.get("parentColumn").asText(), "metrics"); + assertTrue(json.get("materializedChild").asBoolean()); + assertEquals(json.get("sparseKeys").get(1).asText(), "latencyMs"); + } + + /// An ordinary column carries none of those, so every accessor reports absence and the JSON keeps its null slots. + @Test + public void rarelyPopulatedFieldsAbsentByDefault() { + ColumnMetadataImpl metadata = ColumnMetadataImpl.fromPropertiesConfiguration(baseConfig("col"), 1, "col"); + assertNull(metadata.getPartitionFunction()); + assertNull(metadata.getPartitions()); + assertNull(metadata.getParentColumn()); + assertFalse(metadata.isMaterializedChild()); + assertNull(metadata.getSparseKeys()); + JsonNode json = JsonUtils.objectToJsonNode(metadata); + for (String key : List.of("partitionFunction", "partitions", "parentColumn", "sparseKeys")) { + assertTrue(json.get(key).isNull(), key); + } + assertFalse(json.get("materializedChild").asBoolean()); + assertEquals(metadata, ColumnMetadataImpl.fromPropertiesConfiguration(baseConfig("col"), 1, "col")); + assertNotEquals(metadata, fullyPopulated().build()); + } + + /// The six booleans and the forward-index encoding share one byte: each must round-trip independently of the + /// others and take part in equality, hashCode and toString. + @Test + public void flagsRoundTripIndependently() { + FieldSpec fieldSpec = new DimensionFieldSpec("col", DataType.STRING, true); + ColumnMetadataImpl none = ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).build(); + assertEquals(flags(none), Set.of()); + assertEquals(none.getForwardIndexEncoding(), EncodingType.RAW); + + Map single = Map.of( + "sorted", ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).setSorted(true), + "nonNull", ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).setNonNull(true), + "minMaxValueInvalid", ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).setMinMaxValueInvalid(true), + "ascii", ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).setAscii(true), + "autoGenerated", ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).setAutoGenerated(true), + "dictionaryEncodedForwardIndex", + ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).setForwardIndexEncoding(EncodingType.DICTIONARY)); + single.forEach((flag, builder) -> { + ColumnMetadataImpl metadata = builder.build(); + assertEquals(flags(metadata), Set.of(flag)); + assertEquals(metadata, builder.build(), flag); + assertEquals(metadata.hashCode(), builder.build().hashCode(), flag); + assertNotEquals(metadata, none, flag); + assertNotEquals(metadata.hashCode(), none.hashCode(), flag); + }); + // A dictionary without an explicit encoding canonicalizes to a dictionary-encoded forward index + ColumnMetadataImpl dictionary = ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).setHasDictionary(true).build(); + assertEquals(flags(dictionary), Set.of("hasDictionary", "dictionaryEncodedForwardIndex")); + ColumnMetadataImpl sharedDictionary = ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).setHasDictionary(true) + .setForwardIndexEncoding(EncodingType.RAW).build(); + assertEquals(flags(sharedDictionary), Set.of("hasDictionary")); + assertNotEquals(sharedDictionary, dictionary); + + ColumnMetadataImpl all = ColumnMetadataImpl.builder().setFieldSpec(fieldSpec).setHasDictionary(true) + .setSorted(true).setNonNull(true).setMinMaxValueInvalid(true).setAscii(true).setAutoGenerated(true).build(); + assertEquals(flags(all), Set.of("hasDictionary", "dictionaryEncodedForwardIndex", "sorted", "nonNull", + "minMaxValueInvalid", "ascii", "autoGenerated")); + String string = all.toString(); + for (String field : List.of("_hasDictionary=true", "_forwardIndexEncoding=DICTIONARY", "_sorted=true", + "_nonNull=true", "_minMaxValueInvalid=true", "_isAscii=true", "_autoGenerated=true")) { + assertTrue(string.contains(field), string); + } + assertTrue(none.toString().contains("_hasDictionary=false, _forwardIndexEncoding=RAW, _sorted=false"), + none.toString()); + } + + private static Set flags(ColumnMetadataImpl metadata) { + Set flags = new TreeSet<>(); + if (metadata.hasDictionary()) { + flags.add("hasDictionary"); + } + if (metadata.getForwardIndexEncoding() == EncodingType.DICTIONARY) { + flags.add("dictionaryEncodedForwardIndex"); + } + if (metadata.isSorted()) { + flags.add("sorted"); + } + if (metadata.isNonNull()) { + flags.add("nonNull"); + } + if (metadata.isMinMaxValueInvalid()) { + flags.add("minMaxValueInvalid"); + } + if (metadata.isAscii()) { + flags.add("ascii"); + } + if (metadata.isAutoGenerated()) { + flags.add("autoGenerated"); + } + return flags; + } + + private static final PartitionFunction PARTITION_FUNCTION = new TestPartitionFunction(4); + + /// A builder with every field populated, including the rarely set ones and the compression stats. + private static ColumnMetadataImpl.Builder fullyPopulated() { + return ColumnMetadataImpl.builder() + .setFieldSpec(new DimensionFieldSpec("metrics$cpu", DataType.STRING, true)) + .setTotalDocs(10) + .setCardinality(5) + .setHasDictionary(true) + .setSorted(true) + .setNonNull(true) + .setMinValue("a") + .setMaxValue("z") + .setLengthOfShortestElement(1) + .setLengthOfLongestElement(3) + .setAscii(true) + .setBitsPerElement(3) + .setAutoGenerated(true) + .setPartitionFunction(PARTITION_FUNCTION) + .setPartitions(new TreeSet<>(Set.of(1, 3))) + .setParentColumn("metrics") + .setSparseKeys(List.of("region", "latencyMs")) + .setRawForwardIndexUncompressedValueSizeInBytes(4096) + .setRawForwardIndexChunkCompressionType(ChunkCompressionType.LZ4) + .setDictionaryEncodedUncompressedValueSizeInBytes(2048); + } + + /// Minimal [PartitionFunction] (the real ones live in pinot-common) with value equality on the partition count. + private static final class TestPartitionFunction implements PartitionFunction { + private final int _numPartitions; + + private TestPartitionFunction(int numPartitions) { + _numPartitions = numPartitions; + } + + @Override + public int getPartition(String value) { + return Math.floorMod(value.hashCode(), _numPartitions); + } + + @Override + public String getName() { + return "TestPartitionFunction"; + } + + @Override + public int getNumPartitions() { + return _numPartitions; + } + + @Override + public PartitionIdNormalizer getPartitionIdNormalizer() { + return PartitionIdNormalizer.POSITIVE_MODULO; + } + + @Override + public boolean equals(Object o) { + return o instanceof TestPartitionFunction && _numPartitions == ((TestPartitionFunction) o)._numPartitions; + } + + @Override + public int hashCode() { + return _numPartitions; + } + + @Override + public String toString() { + return "TestPartitionFunction{" + _numPartitions + '}'; + } + } + // The index-size API works on numeric index ids so this module's tests need no index plugins registered. private static final short FORWARD_ID = 2; private static final short DICTIONARY_ID = 0;