xiangfu0 commented on code in PR #19486:
URL: https://github.com/apache/pinot/pull/19486#discussion_r4102589564
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java:
##########
@@ -114,11 +128,27 @@ default NavigableSet<String> getAllColumns() {
return getSchema().getColumnNames();
}
+ /// Number of columns in [#getAllColumns()].
+ ///
+ /// A segment that holds no column metadata (a CONSUMING one, built from an
explicit schema) still reports its
+ /// schema's columns here, so this is not the size of
[#getAllColumnMetadata()]: do not pair the two.
+ default int getNumColumns() {
+ return getColumnMetadataMap().size();
+ }
+
/// The column metadata of every column that has some, in the natural
column-name order of [#getAllColumns()], and
/// empty for a segment that holds none (a CONSUMING one, which answers
[#getColumnMetadataFor(String)] with `null`
/// for every column of its schema).
default Collection<ColumnMetadata> getAllColumnMetadata() {
- return getColumnMetadataMap().values();
+ TreeMap<String, ColumnMetadata> columnMetadataMap = getColumnMetadataMap();
Review Comment:
Landed one PR down the stack, in #19481 (15f6c8a955):
`getColumnMetadataMap()` is `@Nullable` with the CONSUMING-segment contract
stated, and the defaults built on it answer a null map with an empty
collection, `null` or an `IllegalStateException`. This PR's null guard on
`getAllColumnMetadata()` merged into that.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java:
##########
@@ -315,25 +321,60 @@ public void testOpenStructChildSpecsSharedButParentIsNot()
@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);
+ }
+
+ /// The preprocess that runs on every segment load asks the forward-index
handler which physical columns exist. That
+ /// question must not build the per-segment schema: doing so once per
segment pins one [Schema] per loaded segment
+ /// for its whole life, which on a server holding tens of thousands of wide
segments is hundreds of megabytes.
+ @Test
+ public void testPreprocessDoesNotBuildTheSegmentSchema()
+ throws Exception {
+ // The forward-index handler skips segments older than v3, so the
preprocess only reaches it on a v3 segment.
+ new SegmentV1V2ToV3FormatConverter().convert(_segmentDirectory);
+
+ long materializations = SegmentMetadataImpl.getNumSchemaMaterializations();
+ SegmentMetadataImpl metadata = new SegmentMetadataImpl(_segmentDirectory);
+ Set<String> physical = metadata.getPhysicalColumnNames();
+ assertFalse(metadata.isSchemaMaterialized(), "listing physical columns
must not build the segment schema");
+ assertEquals(SegmentMetadataImpl.getNumSchemaMaterializations(),
materializations);
+ assertEquals(List.copyOf(physical),
List.copyOf(metadata.getSchema().getPhysicalColumnNames()),
+ "the derived names must equal what the schema reports, in the same
order");
+ assertFalse(physical.contains(BuiltInVirtualColumn.DOCID));
Review Comment:
Moot after the follow-up commits: this PR no longer adds
`getPhysicalColumnNames()` or that test (the forward-index handler iterates the
column metadata directly), so the vacuous assertion is gone. The virtual-column
exclusion on a loaded segment is covered by
`testSchemaIncludesBuiltInVirtualColumnsAfterLoad`, and the server-path test
added in #19478 now also asserts here that the preprocess check leaves the
schema unbuilt.
--
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]
