xiangfu0 commented on code in PR #19511:
URL: https://github.com/apache/pinot/pull/19511#discussion_r4102660188
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPruner.java:
##########
@@ -222,4 +213,18 @@ private boolean checkMinMaxRange(DataSourceMetadata
dataSourceMetadata, Comparab
}
return true;
}
+
+ /// 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.
+ private static DataSourceMetadata getDataSourceMetadata(IndexSegment
segment, String column,
+ Map<String, DataSource> dataSourceCache, QueryContext query) {
+ if (segment instanceof ImmutableSegment) {
Review Comment:
Addressed in 87adad5d75:
`ColumnValueSegmentPrunerTest#testImmutableSegmentIsPrunedFromDataSourceMetadataOnly`
mocks an `ImmutableSegment` with stubbed `getDataSourceMetadata` (INT, min 10
/ max 20, Modulo(5) partition holding 2), asserts the EQ / RANGE / IN /
partition decisions, and verifies `getDataSource` is never called in either
overload.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPruner.java:
##########
@@ -222,4 +213,18 @@ private boolean checkMinMaxRange(DataSourceMetadata
dataSourceMetadata, Comparab
}
return true;
}
+
+ /// Pruning reads only the column's statistics, so an immutable segment
answers from its column metadata rather
Review Comment:
Addressed in 87adad5d75: the Javadoc now gives the real reason
(`MutableSegmentImpl.getDataSourceNullable` builds a new data source per call;
the cache keeps that to one per column and query).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -461,6 +461,20 @@ public SegmentMetadataImpl getSegmentMetadata() {
return _segmentMetadata;
}
+ /// Answers from the column metadata, so a caller that needs only the
column's statistics does not materialize the
+ /// column. That matters most under lazy column materialization: segment
pruning asks this for every segment the
+ /// 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.
+ @Override
+ public DataSourceMetadata getDataSourceMetadata(String column, Schema
schema) {
+ ColumnMetadata columnMetadata =
_segmentMetadata.getColumnMetadataFor(column);
Review Comment:
Addressed in 87adad5d75, option (b) plus one more case: 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; a MAP column dispatches to
`ImmutableMapDataSource.metadataOf`, an OPEN_STRUCT parent to
`ImmutableOpenStructDataSource.metadataOf`, a materialized child or an absent
column falls back to `getDataSource` as before.
`ImmutableSegmentImplTest#testDataSourceMetadataMatchesTheDataSourceKindWithoutMaterializing`
covers each kind and still verifies no interaction with the materializer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
