xiangfu0 commented on code in PR #19475:
URL: https://github.com/apache/pinot/pull/19475#discussion_r4101823839
##########
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:
Addressed in 1a1847a79e. The constructor now walks the numeric ids in order
(`allIndexes.get(indexId)`) instead of iterating the index types and looking
each id up, so `getNumericId` no longer runs during construction. The reader
goes into `readersById[indexId]` on the line right after `createIndexReader`
returns, with nothing in between that can throw.
##########
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:
Keeping the check per construction. A one-time check on first use would
still fire first at the first segment load, since the class initializes lazily
on that load and not at server start. Every later load would still have to
refuse to build a container, so the failure would come no earlier; it would
only skip one `List.size()` comparison per column. With the per-construction
`checkState`, every failed load reports the same clear message. Failing at
startup would need an `IndexService` or server-starter hook that knows this
class's mask width, which isn't worth it while OSS registers 13 of the 64 ids.
--
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]
