xiangfu0 commented on code in PR #19478:
URL: https://github.com/apache/pinot/pull/19478#discussion_r4091517401
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -478,12 +490,12 @@ public DataSource getDataSource(String column, Schema
schema) {
@Override
public Set<String> getColumnNames() {
- return _segmentMetadata.getSchema().getColumnNames();
+ return _columnNames;
Review Comment:
**MINOR (compat note for the description):** `getPhysicalColumnNames()`
changes from a fresh mutable `TreeSet` per call (`Schema.java:390-398`) to a
shared unmodifiable view, and `getColumnNames()` from a live mutable key set to
an unmodifiable one. No in-repo caller mutates either and `IndexSegment`
promises only "Set of column names", but external code that mutated the
returned set now gets `UnsupportedOperationException`. Worth one line in the PR
description.
##########
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:
**MINOR here, but it changes the blast radius of a pre-existing master
bug:** this returns the live, mutable `navigableKeySet()` of
`_columnMetadataMap`. A caller's `remove` bypasses `removeColumn`'s cache
invalidation and time-column guard. `ImmutableSegmentImpl` already wraps the
same key set unmodifiable (`:160`, `:244`); do the same here.
Why it matters now: `TablesResource` (`GET
/tables/{table}/metadata?columns=*`, `:253-257`) does `columnSet =
segmentMetadata.getAllColumns()` for the first immutable segment and then
`columnSet.retainAll(...)` per later segment, which removes through the
iterator. On master that deletes FieldSpecs from the first segment's own
`Schema` (the SPI default returns `Schema._fieldSpecMap.navigableKeySet()`),
and `SELECT *` then silently drops columns for that segment
(`SelectionOperatorUtils:91` expands from `getColumnNames()`). At this head the
same call deletes `ColumnMetadata` entries from a serving segment instead.
#19481 fixes the endpoint with a `new HashSet<>()` copy; that two-line fix
deserves its own hotfix PR against master, merged before this one, with the
stack rebased on it.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/PhysicalColumnNames.java:
##########
@@ -0,0 +1,103 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.indexsegment.immutable;
+
+import java.util.AbstractSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.NoSuchElementException;
+import java.util.SortedMap;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+
+
+/// Unmodifiable view of the physical columns of an immutable segment: the
keys of its column metadata map whose field
+/// spec is not produced by a virtual column provider, in the map's (sorted)
key order.
+///
+/// It is a view rather than a copy so a segment retains nothing per column
for it: the segment schema this replaces
+/// held a `TreeMap` entry per column, and a cached `TreeSet` would hold the
same. `contains` is one map lookup and
+/// iteration is a filtered pass over the map. The virtual column count is
taken once at construction, which is sound
Review Comment:
**MINOR (doc):** "sound because the column metadata map is fixed once the
segment is loaded" is true only because `removeColumn`'s sole caller
(`ImmutableSegmentLoader.java:232`) precedes segment construction; `size()`
silently drifts if that ordering ever changes. Say so in the Javadoc (or
compute the virtual count lazily).
##########
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:
**MAJOR [BUG-CORR]:** this `put` goes into the live
`getColumnMetadataMap()`, and nothing but `removeColumn`
(`SegmentMetadataImpl.java:557-561`) clears the cached `_schema`. On the server
path the same cached `SegmentMetadataImpl` serves both the preprocess check and
the load: `BaseTableDataManager:1239` (`!needPreprocess(segmentDirectory, …)`)
→ `:1183 load(segmentDirectory, …)`, and `:1625 needPreprocess(...)` → `:1635
load(...)`. `needPreprocess` → `SegmentPreProcessor.needProcess()` →
`ForwardIndexHandler.needUpdateIndices` → `computeOperations` →
`segmentMetadata.getSchema().getPhysicalColumnNames()`
(`ForwardIndexHandler.java:251`) runs on
`segmentDirectory.getSegmentMetadata()`, i.e. this instance, *before* the
virtual columns are registered here.
Consequences at this head: (1) every v3 segment loaded that way retains a
materialized `Schema`, so "a normal segment load never materializes it" holds
only for the `load(File, …)` path the tests exercise (that path uses a separate
check directory, loader `:150-157`); (2) the retained schema lacks `$docId` /
`$hostName` / `$segmentName`, whereas the old eager schema was mutated in place
by `addBuiltInVirtualColumnsToSegmentSchema`, so `getSchema().getColumnNames()`
and `getAllColumns()` disagree for a served segment. No in-repo post-load
consumer needs the virtual specs, so no wrong result is demonstrated; external
callers of `getSegmentMetadata().getSchema()` would see the difference.
#19481 fixes (2) — its `addColumnMetadata` calls `invalidateDerivedViews()`
— and #19486 fixes (1) by removing the `ForwardIndexHandler` caller. Please
move the invalidating `addColumnMetadata` (or a package-visible
`invalidateSchema()`) into this PR and call it from here, and add a test on the
`SegmentDirectory` + `needPreprocess` path (v3 segment, table config + schema
set) asserting `!isSchemaMaterialized()` after load, or at minimum that
`getSchema()` after load contains the built-in virtual columns even when it was
called before load.
##########
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:
**MINOR (doc accuracy):** the Javadoc says nothing on the load or query path
should call `getSchema()`, but at this head `ForwardIndexHandler.java:251`,
`ColumnMinMaxValueGenerator.java:99`, `StarTreeV2BuilderConfig.java:123` and
`BaseSingleTreeBuilder.java:157` still do. #19486 removes the first two; the
star-tree builder callers remain (only with dynamic/default star-tree creation
enabled). Fine if the description notes the ordering.
--
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]
