xiangfu0 commented on code in PR #19475:
URL: https://github.com/apache/pinot/pull/19475#discussion_r4106657622
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java:
##########
@@ -67,12 +83,19 @@ 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();
+ checkState(numIndexTypes <= Long.SIZE,
Review Comment:
Done in 9e58933a11: `IndexService.MAX_INDEX_TYPES` (64) is now part of the
SPI, documented on `IndexService` and `IndexPlugin` along with where it comes
from. The `IndexService` constructor rejects a larger plugin set, the per-load
check in the container is gone, and the container and the constant reference
each other. `BaseServerStarter.start()` now loads `IndexService` before joining
the cluster, so an oversized plugin set fails server startup.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java:
##########
@@ -67,12 +83,19 @@ 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();
+ checkState(numIndexTypes <= Long.SIZE,
+ "Cannot track %s index types in a %s-bit presence mask, column: %s",
numIndexTypes, Long.SIZE, columnName);
+ // Scratch array indexed by numeric id; compacted into the exactly-sized
_readers below.
+ IndexReader[] readersById = new IndexReader[numIndexTypes];
+ long presentMask = 0L;
boolean forwardIndexOnly = indexLoadingConfig.isForwardIndexOnly();
try {
- for (IndexType<?, ?, ?> indexType :
IndexService.getInstance().getAllIndexes()) {
+ for (int indexId = 0; indexId < numIndexTypes; indexId++) {
Review Comment:
Done in 9e58933a11: the constructor now gets each id from
`indexService.getNumericId(indexType)`, the same call `getIndex()` uses, before
it creates the reader. `IndexService` also documents that numeric ids are
positions in `getAllIndexes()`.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java:
##########
@@ -119,7 +161,9 @@ public VectorIndexConfig getVectorIndexConfig() {
public void close()
throws IOException {
// TODO (index-spi): Verify that readers can be closed in any order
- _indexTypeMap.close();
+ for (IndexReader reader : _readers) {
Review Comment:
Done in 9e58933a11: `close()` now closes every reader, rethrows the first
failure and attaches later ones as suppressed.
`testCloseClosesRemainingReadersAfterFailure` covers this.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainerTest.java:
##########
@@ -129,17 +130,24 @@ public void testCreateSegmentAndCheckColumnIndexes()
assertNotNull(segment.getIndex(STR_COL, StandardIndexes.json()));
Review Comment:
Done in 9e58933a11: the segment test now checks the type of each present
reader (`JsonIndexReader`, `Dictionary`, `ForwardIndexReader`,
`RangeIndexReader`). I checked it against a deliberately wrong-slot `getIndex`:
the test fails because the json lookup on `STR_COL` returns the sorted forward
reader.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainerTest.java:
##########
@@ -129,17 +130,24 @@ public void testCreateSegmentAndCheckColumnIndexes()
assertNotNull(segment.getIndex(STR_COL, StandardIndexes.json()));
assertNotNull(segment.getIndex(STR_COL, StandardIndexes.dictionary()));
assertNotNull(segment.getIndex(STR_COL, StandardIndexes.forward()));
+ assertNull(segment.getIndex(STR_COL, StandardIndexes.range()));
assertNotNull(segment.getIndex(FLOAT_COL, StandardIndexes.dictionary()));
assertNotNull(segment.getIndex(FLOAT_COL, StandardIndexes.forward()));
assertNotNull(segment.getIndex(FLOAT_COL, StandardIndexes.range()));
+ assertNull(segment.getIndex(FLOAT_COL, StandardIndexes.json()));
assertNotNull(segment.getIndex(DOUBLE_COL,
StandardIndexes.dictionary()));
assertNotNull(segment.getIndex(DOUBLE_COL, StandardIndexes.forward()));
+ assertNull(segment.getIndex(DOUBLE_COL, StandardIndexes.range()));
assertNotNull(segment.getIndex(LONG_COL, StandardIndexes.dictionary()));
assertNotNull(segment.getIndex(LONG_COL, StandardIndexes.forward()));
assertNotNull(segment.getIndex(LONG_COL, StandardIndexes.range()));
+
+ // Sparse index ids must resolve absent readers without shifting the
present ones.
Review Comment:
Done in 9e58933a11: added stub-`IndexService` tests for a reader at id 63
with 64 types, a column with no readers, the `forwardIndexOnly` skip, null and
constraint-violation readers, and cleanup on close and on init failure. The
more-than-64 case moved to `IndexServiceTest`, since `IndexService` now
enforces the limit.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
