xiangfu0 commented on code in PR #19486:
URL: https://github.com/apache/pinot/pull/19486#discussion_r4091529495
##########
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:
**MINOR [C6.5] (vacuous assertion):** `metadata` here is `new
SegmentMetadataImpl(_segmentDirectory)`, which loads physical columns only
("here we only add physical columns", `SegmentMetadataImpl:271-279`), so no
virtual column is registered and this `assertFalse` cannot fail. The
metadata-level exclusion is exercised only through Mockito in
`SegmentMetadataDefaultsTest:54-62`, and the loaded-segment test
`testSchemaIncludesBuiltInVirtualColumnsAfterLoad` asserts
`segment.getPhysicalColumnNames()` (the `ImmutableSegmentImpl` cached field),
not this accessor. Drop this line and, in that loaded-segment test, add after
load
`assertFalse(metadata.getPhysicalColumnNames().contains(BuiltInVirtualColumn.DOCID))`
and `assertEquals(metadata.getPhysicalColumnNames(),
segment.getPhysicalColumnNames())` — there `$docId` / `$hostName` /
`$segmentName` really are in the column metadata, and the call must still leave
`isSchemaMaterialized()` false.
##########
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:
**MINOR [C7.2] (contract consistency):** this default is now null-tolerant
and `SegmentMetadataDefaultsTest` says `getColumnMetadataMap()` "is documented
to answer with `null`", but on the interface `getColumnMetadataMap()` (`:163`)
carries no `@Nullable` and its Javadoc never mentions null — only
`SegmentMetadataImpl` documents it. The sibling defaults `getNumColumns()`,
`forEachColumn`, `getColumnMetadataFor` and `addColumnMetadata` still
dereference the map unguarded, so the interface now has one null-safe default
and four that NPE on the same input. Nothing in-tree can hit it
(`SegmentMetadataImpl` overrides them all); it is a contract gap. Add
`@Nullable` plus one sentence ("`null` for an implementation that holds no
column metadata, e.g. a CONSUMING segment") to `getColumnMetadataMap()`, and
either null-guard the four siblings the same way or state on each that it
requires a map; point the test Javadoc at the interface.
--
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]
