xiangfu0 commented on code in PR #19481:
URL: https://github.com/apache/pinot/pull/19481#discussion_r4091529166
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -535,17 +621,113 @@ public String getEndOffset() {
return _endOffset;
}
+ /// {@inheritDoc}
+ ///
+ /// Built from the column arrays on the first call and cached until the
columns change, so a caller pays one map
+ /// entry per column and the segment keeps it for its lifetime. Nothing on
the load or query path should call this
+ /// — see the accessors listed on [SegmentMetadata#getColumnMetadataMap()].
Writes to the returned map do not reach
+ /// the segment metadata; use [#addColumnMetadata(String, ColumnMetadata)]
and [#removeColumn(String)] instead.
+ ///
+ /// Returns `null` for a CONSUMING segment, which holds no column metadata.
+ @Nullable
@Override
public TreeMap<String, ColumnMetadata> getColumnMetadataMap() {
Review Comment:
**MAJOR [C1.3 / C5.22]:** before, this returned the live backing `TreeMap`
and every other accessor derived from it, so both a `put` into the map and a
subclass override were honoured everywhere. Now the arrays are authoritative
and this is a lazily built, cached, plain mutable `TreeMap`. Two consequences:
1. A caller writing into the returned map (the pattern
`ImmutableSegmentLoader:334` itself used at base) gets a silently inconsistent
object: the write shows through the cached map but not through
`getColumnMetadataFor` / `getAllColumns` / `getSchema` / `toJson`. Nothing
fails.
2. Subclass overrides are bypassed. startree-pinot's
`DedupSnapshotCreationTaskUtils` and `UpsertSnapshotCreationTaskUtils` override
`getColumnMetadataMap()` to return an empty `TreeMap` so the eager
`ImmutableSegmentImpl` constructor sees no columns; the constructor now reads
`getAllColumns()` / `getNumColumns()` / `forEachColumn()`, so the override is
dead (details in the review body).
Fix: make the derived map fail loudly on writes (an anonymous `TreeMap`
subclass throwing from `put` / `putAll` / `remove` / `clear` / `putIfAbsent` /
`compute*` / `merge` / `replace*` / `pollFirstEntry` / `pollLastEntry`;
iterator removal cannot be intercepted, so keep the Javadoc), or return a fresh
copy per call; state in the SPI Javadoc and the PR description that
`getColumnMetadataMap()` is a derived view and overrides no longer feed other
accessors; and file the startree-pinot follow-up.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java:
##########
@@ -113,13 +125,53 @@ 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();
Review Comment:
**MINOR:** the new defaults `getNumColumns()`, `getAllColumnMetadata()` and
`forEachColumn()` NPE when `getColumnMetadataMap()` returns `null`, while their
Javadoc promises schema columns / empty.
`PartitionIdVirtualColumnProvider.java:89-91` also dropped the base null guard
when it switched to `forEachColumn`, so an external implementer returning
`null` for consuming segments (the pre-PR convention) would NPE there. None
exists today in pinot or startree-pinot `src/main`. Make the defaults
null-tolerant (#19486 does it for `getAllColumnMetadata` only; better to do all
of them here).
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java:
##########
@@ -252,15 +252,17 @@ public String getSegmentMetadata(
Set<String> allSegmentColumns = segmentMetadata.getAllColumns();
if (columnSet == null) {
- columnSet = allSegmentColumns;
+ // Copy: getAllColumns() is an unmodifiable view of the
segment's own columns, and retainAll below
+ // would otherwise narrow the first segment's metadata rather
than the running intersection.
+ columnSet = new HashSet<>(allSegmentColumns);
Review Comment:
**MAJOR [PROC] — this is a real fix, and it should land first.** With
`columns=*` this endpoint captured the first immutable segment's
`getAllColumns()` and then `retainAll`'d every later segment into it, which
removes through the iterator, and the set was live:
- master / stack base: `SegmentMetadata.getAllColumns()` defaults to
`getSchema().getColumnNames()` = `Schema._fieldSpecMap.navigableKeySet()`. The
endpoint deletes FieldSpecs from the first segment's own `Schema` for every
column a later segment lacks (the normal case after schema evolution).
`ImmutableSegmentImpl.getColumnNames()` / `getPhysicalColumnNames()` read that
schema live and `SelectionOperatorUtils:91` expands `SELECT *` from it, so that
segment silently returns fewer columns until reload. Unchanged on
upstream/master today; the controller forwards the caller's `columns` list
verbatim, so `GET /tables/{table}/metadata?columns=*` on the controller
triggers it.
- #19478 / #19479 heads: `getAllColumns()` returns
`_columnMetadataMap.navigableKeySet()`, so the same call deletes
`ColumnMetadata` from a serving segment.
- here: fixed by the copy, and `getAllColumns()` is now an unmodifiable
snapshot (would throw rather than corrupt), pinned by
`SegmentMetadataImplTest.testGetAllColumnsIsAnUnmodifiableSnapshot`.
Please lift this copy into a standalone hotfix PR against master with a
regression test (two segments with different column sets, call the endpoint
with `*`, assert the first segment's `getColumnNames()` is unchanged), merge
it, and rebase the stack on it — then no PR in the stack carries the window.
Also call the user-visible fix out in this PR's description.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -436,11 +473,60 @@ public boolean isSchemaMaterialized() {
return _schema != null;
}
- /// 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.
+ /// 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();
+ }
+
+ /// An unmodifiable view of the sorted column name array, 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. The view is a snapshot: it does not reflect columns added or
removed after this call.
@Override
public NavigableSet<String> getAllColumns() {
Review Comment:
**MINOR (pre-existing):** the Javadoc says "unmodifiable view … falls back
to the explicit schema", but the CONSUMING fallback
`getSchema().getColumnNames()` is the live, mutable `navigableKeySet()` of the
shared table `Schema` for `MutableSegmentImpl`. Wrap it in
`Collections.unmodifiableNavigableSet(...)` so both branches honour the
contract.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SortedStringArraySet.java:
##########
@@ -0,0 +1,313 @@
+/**
+ * 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.spi.index.metadata;
+
+import java.util.AbstractSet;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.NavigableSet;
+import java.util.NoSuchElementException;
+import java.util.SortedSet;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+
+/// Unmodifiable [NavigableSet] view of a range of a sorted, duplicate-free
`String[]`, ordered naturally.
+///
+/// Lookups are a binary search over the array, so the whole set costs one
small object rather than a red-black-tree
+/// node per element. That is the point: a server holds the column names of
every loaded segment for the segment's
+/// lifetime, and a wide segment has thousands of them.
+///
+/// The array is referenced, not copied, so the view reflects nothing the
holder does afterwards *except* in-place
+/// writes: [SegmentMetadataImpl] replaces its arrays when its columns change,
which leaves an already-returned view
+/// as the snapshot taken at the time of the call.
+///
+/// `subSet`/`headSet`/`tailSet` are ranges of the same array, and like every
[NavigableSet] range view they reject
+/// an argument outside their own range rather than silently widening it.
+///
+/// Immutable and thread-safe as long as the backing array is not written in
place.
+final class SortedStringArraySet extends AbstractSet<String> implements
NavigableSet<String> {
+ private final String[] _elements;
+ private final int _from;
+ private final int _to;
+ /// The bounds this view was created with, `null` on the side it is
unbounded on. A range view has to reject an
+ /// argument outside its own range, as [NavigableSet] requires, which the
array indices alone cannot tell: an
+ /// exclusive endpoint that is not in the array leaves no trace in them.
+ @Nullable
+ private final String _low;
+ private final boolean _lowInclusive;
+ @Nullable
+ private final String _high;
+ private final boolean _highInclusive;
+
+ SortedStringArraySet(String[] elements) {
+ this(elements, 0, elements.length, null, false, null, false);
+ }
+
+ private SortedStringArraySet(String[] elements, int from, int to, @Nullable
String low, boolean lowInclusive,
+ @Nullable String high, boolean highInclusive) {
+ _elements = elements;
+ _from = from;
+ _to = to;
+ _low = low;
+ _lowInclusive = lowInclusive;
+ _high = high;
+ _highInclusive = highInclusive;
+ }
+
+ @Override
+ public int size() {
+ return _to - _from;
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return _from == _to;
+ }
+
+ @Override
+ public boolean contains(Object o) {
Review Comment:
**MINOR (doc):** `contains(null)` returns `false` here where
`TreeSet.contains(null)` throws `NullPointerException`. Benign (no caller
passes null), but note the difference in the class Javadoc since the class is
documented as a `TreeSet` stand-in.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -253,21 +246,14 @@ public ImmutableSegmentImpl(
/// metadata declares them complex (the same rule the eager constructor
applies).
@Nullable
private static Map<String, List<String>>
groupOpenStructChildren(SegmentMetadataImpl segmentMetadata) {
- Map<String, List<String>> children = null;
- Map<String, ColumnMetadata> columnMetadataMap =
segmentMetadata.getColumnMetadataMap();
- for (Map.Entry<String, ColumnMetadata> entry :
columnMetadataMap.entrySet()) {
- if (entry.getValue() instanceof ColumnMetadataImpl impl &&
impl.isMaterializedChild()) {
- if (children == null) {
- children = new HashMap<>();
- }
- children.computeIfAbsent(impl.getParentColumn(), k -> new
ArrayList<>()).add(entry.getKey());
+ Map<String, List<String>> children = new HashMap<>();
+ segmentMetadata.forEachColumn((column, columnMetadata) -> {
+ if (columnMetadata instanceof ColumnMetadataImpl impl &&
impl.isMaterializedChild()) {
+ children.computeIfAbsent(impl.getParentColumn(), k -> new
ArrayList<>()).add(column);
Review Comment:
**MINOR (nit):** `groupOpenStructChildren` now always allocates the
`HashMap` (base allocated only when an open-struct child existed). Result
identical, load-time only; fine to leave.
--
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]
