vvivekiyer commented on code in PR #9982:
URL: https://github.com/apache/pinot/pull/9982#discussion_r1048958297
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/BaseIndexHandler.java:
##########
@@ -93,6 +100,24 @@ protected void
createForwardIndexIfNeeded(SegmentDirectory.Writer segmentWriter,
_tmpForwardIndexColumns.add(columnName);
}
+ // Update the segmentMetadata
+ File indexDir = _segmentMetadata.getIndexDir();
Review Comment:
Yeah, checked your latest commit and it looks better now. Agreed that we'd
still have to update the object outside the class to reflect the new metadata.
Thanks for the making the change.
##########
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:
Sounds good. Thanks!
--
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]
