xiangfu0 commented on code in PR #19478:
URL: https://github.com/apache/pinot/pull/19478#discussion_r4102165586
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -313,21 +312,26 @@ private static ImmutableSegmentImpl
loadWithLazyColumns(SegmentDirectory segment
starTreeIndexContainer, mcTextReader);
}
- /// Adds the built-in virtual columns to the segment schema and creates
their index containers and metadata.
+ /// Creates the index containers and column metadata of the built-in virtual
columns and registers them in the
+ /// segment metadata. Registering the metadata is what makes the segment
schema include the virtual columns: the
+ /// schema is derived from the column metadata map on demand
([SegmentMetadataImpl#getSchema()]) and is deliberately
+ /// not built here, so a loaded segment retains no per-column schema entries
until something asks for its schema.
+ /// A physical column of the same name wins, as in the schema-based
registration this replaces.
private static void instantiateVirtualColumns(SegmentMetadataImpl
segmentMetadata,
Map<String, ColumnIndexContainer> indexContainerMap) {
Map<String, ColumnMetadata> columnMetadataMap =
segmentMetadata.getColumnMetadataMap();
- Schema segmentSchema = segmentMetadata.getSchema();
-
VirtualColumnProviderFactory.addBuiltInVirtualColumnsToSegmentSchema(segmentSchema,
segmentMetadata.getName());
- for (FieldSpec fieldSpec : segmentSchema.getAllFieldSpecs()) {
- if (fieldSpec.isVirtualColumn()) {
- String columnName = fieldSpec.getName();
- VirtualColumnContext context =
- new VirtualColumnContext(fieldSpec,
segmentMetadata.getTotalDocs(), segmentMetadata);
- VirtualColumnProvider provider =
VirtualColumnProviderFactory.buildProvider(context);
- indexContainerMap.put(columnName,
provider.buildColumnIndexContainer(context));
- columnMetadataMap.put(columnName, provider.buildMetadata(context));
+ String segmentName = segmentMetadata.getName();
+ for (BuiltInVirtualColumnDefinitions.Definition definition :
BuiltInVirtualColumnDefinitions.DEFINITIONS) {
+ String columnName = definition.getName();
+ if (columnMetadataMap.containsKey(columnName)) {
+ continue;
}
+ FieldSpec fieldSpec =
VirtualColumnProviderFactory.createBuiltInFieldSpec(definition, segmentName);
+ VirtualColumnContext context =
+ new VirtualColumnContext(fieldSpec, segmentMetadata.getTotalDocs(),
segmentMetadata);
+ VirtualColumnProvider provider =
VirtualColumnProviderFactory.buildProvider(context);
+ indexContainerMap.put(columnName,
provider.buildColumnIndexContainer(context));
+ columnMetadataMap.put(columnName, provider.buildMetadata(context));
Review Comment:
Addressed in 49a54b2cbc. The loader registers the built-in virtual columns
through `SegmentMetadataImpl.addColumnMetadata`, which drops the derived schema
under the same monitor `getSchema()` builds under (`removeColumn` takes it
too).
`SegmentMetadataImplTest#testServerLoadPathSchemaIncludesVirtualColumnsRegisteredAfterThePreprocessCheck`
drives the server sequence — `needPreprocess(segmentDirectory, …)` then
`load(segmentDirectory, …)` on a v3 segment — asserts the check derived the
schema, and that the schema handed out after load includes the virtual columns
and equals `getAllColumns()`. With the old map write it fails on exactly that
assertion.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -384,9 +404,56 @@ public SegmentVersion getVersion() {
return _segmentVersion;
}
+ /// {@inheritDoc}
+ ///
+ /// For a metadata-backed segment the schema is built from the column
metadata map on the first call (one
+ /// `FieldSpec` per column, the built-in virtual columns included once the
loader has registered them) and cached
+ /// until [#removeColumn(String)]. Nothing on the load or query path should
call this: a caller there re-inflates
+ /// the per-column schema footprint for every segment it touches. Column
names are available through
+ /// [#getAllColumns()] and field specs through
[#getColumnMetadataFor(String)].
@Override
public Schema getSchema() {
- return _schema;
+ Schema schema = _schema;
+ if (schema == null) {
+ synchronized (this) {
+ schema = _schema;
+ if (schema == null) {
+ schema = buildSchema();
+ _schema = schema;
+ }
+ }
+ }
+ return schema;
+ }
+
+ private Schema buildSchema() {
+ NUM_SCHEMA_MATERIALIZATIONS.incrementAndGet();
+ Schema schema = new Schema();
+ for (ColumnMetadata columnMetadata : _columnMetadataMap.values()) {
+ schema.addField(columnMetadata.getFieldSpec());
+ }
+ return schema;
+ }
+
+ /// Whether [#getSchema()] has been called (and its schema cached) since
construction or the last
+ /// [#removeColumn(String)]. Always `true` for a CONSUMING segment, which is
constructed with its schema.
+ @VisibleForTesting
+ public boolean isSchemaMaterialized() {
+ return _schema != null;
+ }
+
+ /// Number of schemas derived from column metadata so far in this JVM. A
load or query path that leaves this
+ /// unchanged did not build any segment's schema.
+ @VisibleForTesting
+ public static long getNumSchemaMaterializations() {
+ return NUM_SCHEMA_MATERIALIZATIONS.get();
+ }
+
+ /// The keys of the column metadata map, i.e. the same names as
`getSchema().getColumnNames()` without building the
+ /// schema. Falls back to the explicit schema of a CONSUMING segment, which
has no column metadata map.
+ @Override
+ public NavigableSet<String> getAllColumns() {
+ return _columnMetadataMap != null ? _columnMetadataMap.navigableKeySet() :
getSchema().getColumnNames();
Review Comment:
Addressed in 49a54b2cbc: `getAllColumns()` is wrapped unmodifiable on both
branches. The endpoint fix is now its own PR against master, #19665, and this
branch carries the identical two-line copy in `TablesResource` so it has no
window until #19665 lands (the hunk drops out on rebase).
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -384,9 +404,56 @@ public SegmentVersion getVersion() {
return _segmentVersion;
}
+ /// {@inheritDoc}
+ ///
+ /// For a metadata-backed segment the schema is built from the column
metadata map on the first call (one
+ /// `FieldSpec` per column, the built-in virtual columns included once the
loader has registered them) and cached
Review Comment:
Addressed in 49a54b2cbc: the Javadoc now says load- and query-path code
should not call it and names the two remaining load-path callers that #19486
moves off it.
--
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]
