This is an automated email from the ASF dual-hosted git repository. xiangfu0 pushed a commit to branch xiangfu0/data-3221-11-metadata-only-pruning in repository https://gitbox.apache.org/repos/asf/pinot.git
commit 12c6eaa41713ce8d13828b1e927c2a26ff907ca0 Author: Xiang Fu <[email protected]> AuthorDate: Fri Sep 25 15:24:48 2026 +0700 Honour the data-source contract in getDataSourceMetadata and cover the pruner's immutable branch ImmutableSegmentImpl.getDataSourceMetadata answered every column with a view over its ColumnMetadata, which is what an ordinary column's data source carries but not what a MAP column (map metadata: unsorted, no row length), an OPEN_STRUCT parent (synthesized metadata, no statistics) or a materialized child (reachable only through its parent) report through getDataSource. Pruning decisions were unaffected, but the method's contract was not honoured. It now answers a column whose data source already exists with that data source's own metadata, dispatches MAP and OPEN_STRUCT parents to their metadata factories, and falls back to getDataSource for a child or an absent column, still without touching the materializer (ImmutableSegmentImplTest#testDataSourceMetadataMatchesTheDataSourceKindWithoutMaterializing). The pruner's immutable branch had no coverage: ColumnValueSegmentPrunerTest mocked a plain IndexSegment, so the mutable path ran in every case. testImmutableSegmentIsPrunedFromDataSourceMetadataOnly mocks an ImmutableSegment, checks the EQ/RANGE/IN/partition decisions, and verifies getDataSource is never called. The Javadoc on the mutable-segment cache now gives the real reason it is kept. --- .../query/pruner/ColumnValueSegmentPruner.java | 5 +-- .../query/pruner/ColumnValueSegmentPrunerTest.java | 37 ++++++++++++++++++++ .../immutable/ImmutableSegmentImpl.java | 23 ++++++++++--- .../segment/index/map/ImmutableMapDataSource.java | 6 ++++ .../openstruct/ImmutableOpenStructDataSource.java | 7 ++++ .../immutable/ImmutableSegmentImplTest.java | 40 ++++++++++++++++++++++ 6 files changed, 112 insertions(+), 6 deletions(-) diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPruner.java b/pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPruner.java index 741a7cf8c88..7169d71d424 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPruner.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPruner.java @@ -221,8 +221,9 @@ public class ColumnValueSegmentPruner extends ValueBasedSegmentPruner { } /// Pruning reads only the column's statistics, so an immutable segment answers from its column metadata rather - /// than materializing the column. A mutable segment keeps the per-segment data-source cache it had, where the - /// lookup is a map read and the metadata is not derivable without the data source. + /// than materializing the column. A mutable segment keeps the per-segment data-source cache it had: its metadata + /// is not derivable without the data source, and `MutableSegmentImpl` builds a new data source on every call, so + /// the cache is what keeps that to one per column and query. private static DataSourceMetadata getDataSourceMetadata(IndexSegment segment, String column, Map<String, DataSource> dataSourceCache, QueryContext query) { if (segment instanceof ImmutableSegment) { diff --git a/pinot-core/src/test/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPrunerTest.java b/pinot-core/src/test/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPrunerTest.java index d4269141f75..20490685074 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPrunerTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPrunerTest.java @@ -37,6 +37,7 @@ import java.util.concurrent.atomic.AtomicInteger; import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.pinot.core.query.request.context.QueryContext; import org.apache.pinot.core.query.request.context.utils.QueryContextConverterUtils; +import org.apache.pinot.segment.spi.ImmutableSegment; import org.apache.pinot.segment.spi.IndexSegment; import org.apache.pinot.segment.spi.SegmentMetadata; import org.apache.pinot.segment.spi.datasource.DataSource; @@ -53,8 +54,11 @@ import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; @@ -398,6 +402,39 @@ public class ColumnValueSegmentPrunerTest { } } + /// An immutable segment is pruned from its column statistics alone: the pruner asks for the data-source metadata + /// and never for the data source, which under lazy column materialization would build the column's index readers + /// for a segment about to be discarded. Same decisions as the mutable path, reached without a data source. + @Test + public void testImmutableSegmentIsPrunedFromDataSourceMetadataOnly() { + ImmutableSegment segment = mock(ImmutableSegment.class); + when(segment.getColumnNames()).thenReturn(ImmutableSet.of("column")); + SegmentMetadata segmentMetadata = mock(SegmentMetadata.class); + when(segmentMetadata.getTotalDocs()).thenReturn(20); + when(segment.getSegmentMetadata()).thenReturn(segmentMetadata); + DataSourceMetadata metadata = mock(DataSourceMetadata.class); + when(metadata.getDataType()).thenReturn(DataType.INT); + when(metadata.getMinValue()).thenReturn(10); + when(metadata.getMaxValue()).thenReturn(20); + when(metadata.getPartitionFunction()).thenReturn(PartitionFunctionFactory.getPartitionFunction("Modulo", 5, null)); + when(metadata.getPartitions()).thenReturn(Set.of(2)); + when(segment.getDataSourceMetadata(eq("column"), any(Schema.class))).thenReturn(metadata); + + // Min/max: EQ, RANGE and IN + assertTrue(runPruner(segment, "SELECT COUNT(*) FROM testTable WHERE column = 0")); + assertFalse(runPruner(segment, "SELECT COUNT(*) FROM testTable WHERE column = 12")); + assertTrue(runPruner(segment, "SELECT COUNT(*) FROM testTable WHERE column > 20")); + assertFalse(runPruner(segment, "SELECT COUNT(*) FROM testTable WHERE column BETWEEN 15 AND 30")); + assertTrue(runPruner(segment, "SELECT COUNT(*) FROM testTable WHERE column IN (0, 30)")); + assertFalse(runPruner(segment, "SELECT COUNT(*) FROM testTable WHERE column IN (0, 12)")); + // Partition: 12 % 5 = 2 is held, 11 % 5 = 1 is not + assertTrue(runPruner(segment, "SELECT COUNT(*) FROM testTable WHERE column = 11")); + assertFalse(runPruner(segment, "SELECT COUNT(*) FROM testTable WHERE column = 12")); + + verify(segment, never()).getDataSource(anyString(), any(Schema.class)); + verify(segment, never()).getDataSource(anyString()); + } + private QueryContext pruningQuery() { QueryContext query = QueryContextConverterUtils.getQueryContext( "SELECT COUNT(*) FROM testTable WHERE column = 10"); 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 09d1a6316cc..30b16c60d46 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 @@ -467,13 +467,28 @@ public class ImmutableSegmentImpl implements ImmutableSegment { /// server holds, and building an index container per segment there would put a reader — and, for an external /// table, a Parquet footer parse — on the query thread for segments that are about to be pruned away. /// - /// Falls back to the data source for a column the segment does not have, which is where the schema-driven default - /// and virtual columns are created. + /// A column whose data source already exists (every column in eager mode, a materialized one in lazy mode) answers + /// with that data source's own metadata, so the two calls can never disagree. A column that is still to be + /// materialized answers with the metadata its data source would carry: a MAP column's map metadata (unsorted, no + /// row length), an OPEN_STRUCT parent's synthesized metadata (no statistics), and every other column's view over + /// its [ColumnMetadata]. A column the segment does not expose (a materialized OPEN_STRUCT child, or one absent + /// from the segment) falls back to the data source, which is where the schema-driven default and virtual columns + /// are created and where the same error is raised as before. @Override public DataSourceMetadata getDataSourceMetadata(String column, Schema schema) { + DataSource dataSource = _dataSources.get(column); + if (dataSource != null) { + return dataSource.getDataSourceMetadata(); + } ColumnMetadata columnMetadata = _segmentMetadata.getColumnMetadataFor(column); - return columnMetadata != null ? ImmutableDataSource.metadataOf(columnMetadata) - : getDataSource(column, schema).getDataSourceMetadata(); + if (columnMetadata == null || isMaterializedChild(columnMetadata)) { + return getDataSource(column, schema).getDataSourceMetadata(); + } + if (_openStructChildren != null && _openStructChildren.containsKey(column)) { + return ImmutableOpenStructDataSource.metadataOf(columnMetadata.getFieldSpec(), _segmentMetadata.getTotalDocs()); + } + return columnMetadata.getFieldSpec().getDataType() == FieldSpec.DataType.MAP + ? ImmutableMapDataSource.metadataOf(columnMetadata) : ImmutableDataSource.metadataOf(columnMetadata); } @Override diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/ImmutableMapDataSource.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/ImmutableMapDataSource.java index 64d7e8b6c75..3c406e5dd3e 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/ImmutableMapDataSource.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/ImmutableMapDataSource.java @@ -35,6 +35,12 @@ import org.apache.pinot.spi.data.FieldSpec; public class ImmutableMapDataSource extends BaseMapDataSource { private final MapIndexReader _mapIndexReader; + /// The data-source metadata of a MAP column, without a data source: what [#getDataSourceMetadata()] would report, + /// for a caller that must not materialize the column to read its statistics. + public static DataSourceMetadata metadataOf(ColumnMetadata columnMetadata) { + return new ImmutableMapDataSourceMetadata(columnMetadata); + } + public ImmutableMapDataSource(ColumnMetadata columnMetadata, ColumnIndexContainer columnIndexContainer) { super(new ImmutableMapDataSourceMetadata(columnMetadata), columnIndexContainer); MapIndexReader mapIndexReader; diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/ImmutableOpenStructDataSource.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/ImmutableOpenStructDataSource.java index 20fe11ec95c..1087a2a982d 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/ImmutableOpenStructDataSource.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/ImmutableOpenStructDataSource.java @@ -263,6 +263,13 @@ public class ImmutableOpenStructDataSource extends BaseDataSource implements Ope } } + /// The data-source metadata of an OPEN_STRUCT parent column, without a data source: what + /// [#getDataSourceMetadata()] would report (no statistics, unknown cardinality), for a caller that must not + /// materialize the parent's children to read it. + public static DataSourceMetadata metadataOf(FieldSpec fieldSpec, int numDocs) { + return new ImmutableOpenStructDataSourceMetadata(fieldSpec, numDocs); + } + private static class ImmutableOpenStructDataSourceMetadata implements DataSourceMetadata { private final FieldSpec _fieldSpec; private final int _numDocs; 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 4821d0cbe55..87ee7629af2 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 @@ -194,6 +194,46 @@ public class ImmutableSegmentImplTest { segment.destroy(); } + /// The metadata answered without a data source has to be the one the data source would carry: a MAP column's map + /// metadata, an OPEN_STRUCT parent's synthesized metadata, and, once a column is materialized, that data source's + /// own. A materialized child stays reachable only through its parent, so asking for it fails as before. + @Test + public void testDataSourceMetadataMatchesTheDataSourceKindWithoutMaterializing() + throws Exception { + ComplexFieldSpec mapSpec = new ComplexFieldSpec("m", FieldSpec.DataType.MAP, true, + Map.of(ComplexFieldSpec.KEY_FIELD, new DimensionFieldSpec("key", FieldSpec.DataType.STRING, true), + ComplexFieldSpec.VALUE_FIELD, new DimensionFieldSpec("value", FieldSpec.DataType.INT, true))); + 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"); + ColumnMetadataImpl a = columnMetadata(intColumn("a"), null); + ColumnMetadataImpl m = columnMetadata(mapSpec, null); + ColumnMetadataImpl parent = columnMetadata(metrics, null); + ColumnMetadataImpl views = + columnMetadata(new DimensionFieldSpec(viewsColumn, FieldSpec.DataType.LONG, true), "metrics"); + ColumnIndexContainer containerA = mock(ColumnIndexContainer.class); + ColumnMaterializer materializer = mock(ColumnMaterializer.class); + when(materializer.createIndexContainer(a)).thenReturn(containerA); + ImmutableSegmentImpl segment = lazySegment(mock(SegmentDirectory.class), materializer, a, m, parent, views); + Schema schema = mock(Schema.class); + + DataSourceMetadata mapMetadata = segment.getDataSourceMetadata("m", schema); + assertFalse(mapMetadata.isSorted()); + assertThrows(UnsupportedOperationException.class, mapMetadata::getMaxRowLengthInBytes); + DataSourceMetadata parentMetadata = segment.getDataSourceMetadata("metrics", schema); + assertSame(parentMetadata.getFieldSpec(), metrics); + assertNull(parentMetadata.getMinValue()); + assertNull(parentMetadata.getPartitionFunction()); + assertEquals(parentMetadata.getNumDocs(), parent.getTotalDocs()); + assertThrows(IllegalStateException.class, () -> segment.getDataSourceMetadata(viewsColumn, schema)); + verifyNoInteractions(materializer); + + DataSource dataSourceA = segment.getDataSource("a", schema); + assertSame(segment.getDataSourceMetadata("a", schema), dataSourceA.getDataSourceMetadata()); + verify(materializer, times(1)).createIndexContainer(a); + segment.destroy(); + } + @Test public void testLazyModeMaterializesEachColumnOnceUnderConcurrentAccess() throws Exception { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
