xiangfu0 commented on code in PR #19475:
URL: https://github.com/apache/pinot/pull/19475#discussion_r4091516712
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java:
##########
@@ -81,32 +103,52 @@ public
PhysicalColumnIndexContainer(SegmentDirectory.Reader segmentReader, Colum
try {
IndexReader reader =
readerProvider.createIndexReader(segmentReader, fieldIndexConfigs, metadata);
if (reader != null) {
- indexTypes.add(indexType);
- readers.add(reader);
+ short indexId = indexService.getNumericId(indexType);
Review Comment:
**MINOR (hardening, optional):** the reader is created on the line above and
only becomes reachable for cleanup once `readersById[indexId] = reader` runs;
`getNumericId` sits between the two and can throw `IllegalArgumentException`.
Unreachable today (same `IndexService` instance on both sides), and already
strictly better than base where `getNumericId` ran outside the try. Computing
`indexId` before `createIndexReader` makes "every created reader is tracked
before anything else can throw" structural rather than incidental.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java:
##########
@@ -67,12 +83,18 @@ public PhysicalColumnIndexContainer(SegmentDirectory.Reader
segmentReader, Colum
}
_vectorIndexConfig = fieldIndexConfigs.getConfig(StandardIndexes.vector());
- ArrayList<IndexType> indexTypes = new ArrayList<>();
- ArrayList<IndexReader> readers = new ArrayList<>();
+ IndexService indexService = IndexService.getInstance();
+ List<IndexType<?, ?, ?>> allIndexes = indexService.getAllIndexes();
+ int numIndexTypes = allIndexes.size();
Review Comment:
**MINOR (design note, no action needed):** the `> Long.SIZE` guard runs per
(segment, column) construction, so with 65+ registered index types a server
starts cleanly and then fails every segment load with `IllegalStateException`
rather than failing at startup. Placing the guard in `IndexService` would
couple an SPI class to one consumer's mask width, so this placement is
defensible; a one-time static check on first use would be the middle ground.
--
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]
