somandal commented on code in PR #9982:
URL: https://github.com/apache/pinot/pull/9982#discussion_r1048863893
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/InvertedIndexAndDictionaryBasedForwardIndexCreator.java:
##########
@@ -218,20 +218,23 @@ public void regenerateForwardIndex()
LoaderUtils.writeIndexToV3Format(_segmentWriter, _columnName,
_forwardIndexFile, ColumnIndexType.FORWARD_INDEX);
- if (!_isTemporaryForwardIndex) {
- // Only update the metadata and cleanup other indexes if the forward
index to be created is permanent. If the
- // forward index is temporary, it is meant to be used only for
construction of other indexes and will be deleted
- // once all the IndexHandlers have completed.
- try {
- LOGGER.info("Created forward index from inverted index and dictionary.
Updating metadata properties for "
- + "segment: {}, column: {}, property list: {}", segmentName,
_columnName, metadataProperties);
- SegmentMetadataUtils.updateMetadataProperties(_segmentMetadata,
metadataProperties);
- } catch (Exception e) {
- throw new IOException(
- String.format("Failed to update metadata properties for segment:
%s, column: %s", segmentName, _columnName),
- e);
- }
+ try {
+ // Update the metadata even for temporary forward index as other
IndexHandlers may rely on the updated metadata
+ // to construct their indexes based on the forward index.
+ LOGGER.info("Created forward index from inverted index and dictionary.
Updating metadata properties for "
+ + "segment: {}, column: {}, property list: {}, is temporary: {}",
segmentName, _columnName,
+ metadataProperties, _isTemporaryForwardIndex);
+ SegmentMetadataUtils.updateMetadataProperties(_segmentMetadata,
metadataProperties);
+ } catch (Exception e) {
+ throw new IOException(
+ String.format("Failed to update metadata properties for segment: %s,
column: %s", segmentName, _columnName),
+ e);
+ }
+ if (!_isTemporaryForwardIndex) {
Review Comment:
So @Jackie-Jiang and I discussed this in the slack thread where this issue
was brought up and we decided not to reset the metadata to the original one on
deletion of the forward index. The main reason for this is that:
- Temporary forward index should only be created for scenarios where forward
index is disabled, which means dictionary must be present. So metadata changes
to toggle the `hasDictionary` flag will never happen. SV columns only modify
dictionary related metadata when reconstructing the forward index.
- For MV columns we have two additional metadata values that get modified:
`MAX_MULTI_VALUE_ELEMENTS` and `TOTAL_NUMBER_OF_ENTRIES`. These can change if
the MV column has duplicates. We decided that changing these and leaving them
behind in the metadata after deletion should be okay.
Does that make sense? Do you see any other reason to change the metadata
back?
--
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
