siddharthteotia commented on code in PR #9740: URL: https://github.com/apache/pinot/pull/9740#discussion_r1017640531
########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java: ########## @@ -112,20 +122,50 @@ public void updateIndices(SegmentDirectory.Writer segmentWriter, IndexCreatorPro Operation operation = entry.getValue(); switch (operation) { - case CHANGE_RAW_INDEX_COMPRESSION_TYPE: { - rewriteRawForwardIndex(column, segmentWriter, indexCreatorProvider); + case DISABLE_FORWARD_INDEX_FOR_DICT_COLUMN: { + // Deletion of the forward index will be handled outside the index handler to ensure that other index + // handlers that need the forward index to construct their own indexes will have it available. + // The existing forward index must be in dictionary format for this to be a no-op. + _forwardIndexDisabledColumnsToCleanup.add(column); + break; + } + case DISABLE_FORWARD_INDEX_FOR_RAW_COLUMN: { + // The forward index has been disabled for a column which has a noDictionary based forward index. A dictionary + // and inverted index need to be created before we can delete the forward index. We create a dictionary here, + // but let the InvertedIndexHandler handle the creation of the inverted index. We create a temporary + // forward index here which is dictionary based and allow the post deletion step handle the actual deletion + // of the forward index. + createDictBasedForwardIndex(column, segmentWriter, indexCreatorProvider); + Preconditions.checkState(segmentWriter.hasIndexFor(column, ColumnIndexType.FORWARD_INDEX), + String.format("Temporary forward index was not created for column: %s", column)); + _forwardIndexDisabledColumnsToCleanup.add(column); Review Comment: I feel tableConfig will become inconsistent / misleading in this case ? The point being that for disabling the forward index on raw column, we will enforce the constraint of creating dict + inv index. But the tableConfig will continue to have column as noDictionary. We should probably discuss this once and see if we should implicitly update tableConfig, throw exception here to first force the user to change tableConfig or continue with what is done in this PR if we are certain there is no impact of tableConfig not reflecting this change -- 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