gortiz commented on code in PR #19475:
URL: https://github.com/apache/pinot/pull/19475#discussion_r4106483223
##########
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:
This loop uses the position in `getAllIndexes()` as the numeric id, but
`getIndex()` uses `IndexService.getNumericId()`. They are equal today, because
`IndexService` builds `_allIndexPosById` from the same sorted list. The old
code did not depend on that: it called `getNumericId()` while building. If
`IndexService` ever assigns ids differently, lookups return the wrong reader
and nothing reports an error.
Could you use `indexService.getNumericId(indexType)` here, or assert that it
equals `indexId`? It runs only at load time, so it costs nothing on the query
path. (minor)
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java:
##########
@@ -81,32 +104,51 @@ public
PhysicalColumnIndexContainer(SegmentDirectory.Reader segmentReader, Colum
try {
IndexReader reader =
readerProvider.createIndexReader(segmentReader, fieldIndexConfigs, metadata);
if (reader != null) {
- indexTypes.add(indexType);
- readers.add(reader);
+ readersById[indexId] = reader;
+ presentMask |= 1L << indexId;
}
} catch (IndexReaderConstraintException ex) {
LOGGER.warn("Constraint violation when indexing {} with {} index",
columnName, indexType, ex);
}
}
}
} catch (Throwable t) {
- for (IndexReader reader : readers) {
- try {
- reader.close();
- } catch (Throwable ct) {
- LOGGER.warn("Can't close reader on init error, column: " +
columnName + " reader: " + reader.getClass(), ct);
+ for (IndexReader reader : readersById) {
+ if (reader != null) {
+ try {
+ reader.close();
+ } catch (Throwable ct) {
+ LOGGER.warn("Can't close reader on init error, column: " +
columnName + " reader: " + reader.getClass(),
+ ct);
+ }
}
}
throw t;
}
- _indexTypeMap = IndexTypeMap.get(indexTypes, readers);
+ _presentMask = presentMask;
+ int numReaders = Long.bitCount(presentMask);
+ if (numReaders == 0) {
+ _readers = EMPTY_READERS;
+ } else {
+ _readers = new IndexReader[numReaders];
+ int pos = 0;
+ for (IndexReader reader : readersById) {
+ if (reader != null) {
+ _readers[pos++] = reader;
+ }
+ }
+ }
}
@Nullable
@Override
public <I extends IndexReader, T extends IndexType<?, I, ?>> I getIndex(T
indexType) {
- return _indexTypeMap.getIndex(indexType);
+ short indexId = IndexService.getInstance().getNumericId(indexType);
Review Comment:
The PR measures heap and load-time allocation, but not the lookup.
`BaseDataSource` does not cache readers, so every `getDictionary()` /
`getForwardIndex()` / ... call comes here: several times per column per segment
per query. I expect no change or a small gain. The new path has one less
pointer hop, `Long.bitCount` is an intrinsic, and the `getNumericId` hash
lookup that both versions do costs more than the bit math. A small JMH
comparing old and new `getIndex` over present and absent ids would still be
cheap and would support the claim that the cost is unchanged. (minor)
##########
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:
This adds a hard limit to the index SPI: at most 64 distinct index ids
across all `IndexPlugin`s. The SPI classes (`IndexPlugin`, `IndexType`,
`IndexService` in `pinot-segment-spi`) do not mention it, so a plugin author
finds out only when segment loads fail.
Also, this check runs once per column on every segment load. If a deployment
goes over 64 types, the server starts normally and then every segment load
fails with `IllegalStateException`.
Suggestions:
- Document the limit in the `IndexPlugin` / `IndexService` Javadoc. Say that
it comes from the `long` presence mask here, and that more than 64 types needs
a code change (for example a `long[]` mask or a fallback layout).
- Enforce it once, in the `IndexService` constructor, so a bad plugin set
stops the server at startup.
- Link the two places, so that anyone who changes the mask also sees the SPI
contract.
##########
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:
nit, not caused by this PR: if one reader throws here, the remaining readers
do not close. The old code had the same problem. Since you are changing this
method, you could close each reader in its own try/catch, like the init-error
path does. Optional.
##########
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:
All the assertions go through a full segment build with the default index
types. A small unit test with a synthetic `IndexService` (through
`IndexService.setInstance`) could cover the new cases: a reader at bit 63, the
`checkState` failure with more than 64 types, a column with no readers
(`EMPTY_READERS`), the `forwardIndexOnly` skip, and the init-error cleanup over
the sparse scratch array. (minor)
##########
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:
The one way the popcount can fail is to return the reader of a *different*
index, for example the forward reader for `getIndex(json)`. The `(I)` cast is
erased and `assertNotNull` takes an `Object`, so no cast check runs and this
test would still pass. Could you assert the type (`instanceof JsonIndexReader`,
`RangeIndexReader`, `Dictionary`, `ForwardIndexReader`) or use `assertSame`
against a known reader? `STR_COL` is a good case: json has id 7 but sits at
dense position 2.
--
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]
