gortiz commented on code in PR #10184:
URL: https://github.com/apache/pinot/pull/10184#discussion_r1151506168
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java:
##########
@@ -162,22 +165,27 @@ private File getValidDocIdsSnapshotFile() {
}
@Override
- public Dictionary getDictionary(String column) {
+ public <I extends IndexReader> I getIndex(String column, IndexType<?, I, ?>
type) {
ColumnIndexContainer container = _indexContainerMap.get(column);
if (container == null) {
throw new NullPointerException("Invalid column: " + column);
}
- return container.getDictionary();
+ return type.getIndexReader(container);
Review Comment:
That was the original idea, but there are some issues with that, as said in
the javadoc:
```java
/**
* This method is used to extract a compatible reader from a given
ColumnIndexContainer.
*
* Most implementations just return {@link
ColumnIndexContainer#getIndex(IndexType)}, but some may try to reuse other
* indexes. For example, InvertedIndexType delegates on the
ForwardIndexReader when it is sorted.
*/
default IR getIndexReader(ColumnIndexContainer indexContainer) {
return indexContainer.getIndex(this);
}
```
`ColumnIndexContainer` is mostly a map from index type to index reader. When
it is populated, it doesn't know these particularities about inverted index. We
could either add these conditions into `ColumnIndexContainer` or delegate that
decision on the index type. I think doing the former would be against the
single responsibility principle, so I preferred the later.
--
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]
