xiangfu0 commented on code in PR #19511:
URL: https://github.com/apache/pinot/pull/19511#discussion_r4091518151
##########
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:
**MINOR (doc):** the reason given for keeping the mutable cache ("the lookup
is a map read") is not the real one: `MutableSegmentImpl.getDataSourceNullable`
(`:1218-1223`) builds a new `DataSource` per call via
`indexContainer.toDataSource()`, and that allocation is what the cache avoids.
Reword.
##########
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:
**MAJOR [testing]:** this branch is the only query-path behaviour change in
the PR and has no pruner-level coverage.
`ColumnValueSegmentPrunerTest.mockIndexSegment()` returns
`mock(IndexSegment.class)` (`:426`), so `segment instanceof ImmutableSegment`
is false and every assertion in that class runs the mutable/cache branch —
before and after this PR. A future edit re-introducing `getDataSource` here
(re-materializing columns under lazy mode) passes every existing test. The new
`ImmutableSegmentImplTest` case covers the segment override in lazy mode only
and asserts fieldSpec/dataType/numDocs/isSorted, not the min/max/partition
fields the pruner actually reads.
Suggested test: a `ColumnValueSegmentPrunerTest` case with
`mock(ImmutableSegment.class)` stubbing `getDataSourceMetadata(eq("column"),
any(Schema.class))` (min 10 / max 20 / INT plus a partition function),
asserting the same EQ/IN/RANGE/partition decisions as the existing cases and
`verify(segment, never()).getDataSource(anyString(), any(Schema.class))`.
Mockito returns null for an unstubbed default method, so the stub is required —
which is exactly the contract worth pinning.
##########
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:
**MINOR (contract, no wrong result today):** the override does not honour
the new method's documented contract ("the metadata of a column's data source")
for three column classes:
| column class | old (`getDataSource(...).getDataSourceMetadata()`) | new
(override) | reachable via the pruner? |
|---|---|---|---|
| MAP | `ImmutableMapDataSourceMetadata`: `isSorted()==false`,
`getMaxRowLengthInBytes()` throws | delegating view: `isSorted()` delegates
(false in practice), `getMaxRowLengthInBytes()` = -1 | a bare MAP identifier
throws in `DataType.convertInternal` in BOTH paths before min/max is read |
| OPEN_STRUCT parent | synthesized
`ImmutableOpenStructDataSourceMetadata(fieldSpec, numDocs)`: min/max/partition
null, cardinality UNKNOWN | view over the parent's on-disk `ColumnMetadata`
(cardinality/numValues differ; min/max/partition may) | same `ComplexFieldSpec`
→ conversion throws in both paths |
| materialized child `col$$key` | `getDataSourceNullable` returns null
(children only via the parent) → table-schema lookup → `IllegalStateException`
| returns the child's metadata view | only a hand-typed identifier;
`FilterPlanNode.java:293` throws for surviving segments, so the only visible
difference is "empty result" instead of "error" when every segment prunes |
Fix, either: (a) narrow the Javadoc on `IndexSegment.java:75-82` to the
fields pruning reads (data type, min/max, partition info); or (b) dispatch in
the override — `isMaterializedChild(columnMetadata)` → fall through; MAP → an
`ImmutableMapDataSource.metadataOf(columnMetadata)`; OPEN_STRUCT parent → `new
ImmutableOpenStructDataSourceMetadata(fieldSpec, totalDocs)` (needs
visibility). (b) is a few lines and still never touches the materializer; it
matters if `SelectionPlanNode:187` (reads `isSorted`) is migrated later.
--
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]
