xiangfu0 commented on code in PR #19481:
URL: https://github.com/apache/pinot/pull/19481#discussion_r4102583941
##########
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:
Addressed in 15f6c8a955. `getColumnMetadataMap()` now returns a `TreeMap`
subclass that rejects every mutator, including through its key, value, entry
and range views (`SegmentMetadataImplTest#testColumnMetadataMapRejectsWrites`),
so a write fails loudly instead of missing every other accessor; the SPI and
impl Javadoc say it is a derived, read-only view and name `addColumnMetadata` /
`removeColumn` as the way to mutate. The startree-pinot snapshot-task overrides
are called out in the PR description as a follow-up.
##########
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:
Now its own PR against master: #19665, with a regression test over three
segments with pairwise different column sets (fails without the copy: the third
segment loses a column). #19478 carries the identical hunk from here on, so no
PR in the stack has the window; this PR's copy of it drops out on rebase once
#19665 lands.
##########
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:
Addressed in 15f6c8a955: `getColumnMetadataMap()` is `@Nullable` with the
CONSUMING contract stated, `getAllColumnMetadata()` answers an empty list and
`addColumnMetadata` an `IllegalStateException` on a null map (`getNumColumns` /
`forEachColumn` were already dropped from the SPI by the earlier follow-up).
--
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]
