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;