somandal commented on code in PR #9982:
URL: https://github.com/apache/pinot/pull/9982#discussion_r1048859412
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -107,17 +107,20 @@ public void process()
List<IndexHandler> indexHandlers = new ArrayList<>();
for (ColumnIndexType type : ColumnIndexType.values()) {
IndexHandler handler =
- IndexHandlerFactory.getIndexHandler(type, _segmentMetadata,
_indexLoadingConfig, _schema);
+ IndexHandlerFactory.getIndexHandler(type, _segmentMetadata,
_indexLoadingConfig, _schema,
+ _segmentDirectory);
indexHandlers.add(handler);
+ // TODO: Find a way to ensure ForwardIndexHandler is always executed
before other handlers instead of
+ // relying on enum ordering.
handler.updateIndices(segmentWriter, indexCreatorProvider);
- if (type == ColumnIndexType.FORWARD_INDEX) {
- // TODO: Find a way to ensure ForwardIndexHandler is always executed
before other handlers instead of
- // relying on enum ordering.
- // ForwardIndexHandler may modify the segment metadata while
rewriting forward index to create a dictionary
- // This new metadata is needed to construct other indexes like
RangeIndex.
- _segmentMetadata = new SegmentMetadataImpl(indexDir);
- _segmentDirectory.reloadMetadata();
- }
+ // ForwardIndexHandler may modify the segment metadata while rewriting
forward index to create / remove a
+ // dictionary. Other IndexHandler classes may modify the segment
metadata while creating a temporary forward
+ // index to generate their respective indexes from if the forward
index was disabled. This new metadata is
+ // needed to construct other indexes like RangeIndex. This also needs
to be done outside of the IndexHandler
+ // code since modifying the `_segmentMetadata` within the IndexHandler
doesn't modify this object directly but
+ // creates a new one for use within the IndexHandler.
+ _segmentMetadata = new SegmentMetadataImpl(indexDir);
Review Comment:
We will still need the following part:
```
_segmentMetadata = new SegmentMetadataImpl(indexDir);
```
But can remove this part:
```
_segmentDirectory.reloadMetadata();
```
We need the first part because inside the `IndexHandler` since we're
creating a new `SegmentMetadata` object, this object which was passed to the
`IndexHandler` won't be updated but instead a new local `SegmentMetadata`
object will be created. Without re-creating the `SegmentMetadata` object here,
we'll be passing in the older `_segmentMetadata` without the latest changes
made to the on-disk structure. Hope this makes sense. I can remove the
`_segmentDirectory.reloadMetadata()` from here.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
