somandal commented on code in PR #9740:
URL: https://github.com/apache/pinot/pull/9740#discussion_r1018550943
##########
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:
So there is actually no worry here about TableConfig inconsistency. The
TableConfig will have to be updated to disable the forward index. We added a
check in the TableConfigUtils which checks for the following:
```
/**
* Validates the compatibility of the indexes if the column has the
forward index disabled. Throws exceptions due to
* compatibility mismatch. The checks performed are:
* - Validate dictionary is enabled.
* - Validate inverted index is enabled.
* - Validate that either no range index exists for column or the
range index version is at least 2 and isn't a
* multi-value column (since mulit-value defaults to index v1).
*/
private static void validateForwardIndexDisabledIndexCompatibility()
```
I will update this to add back my Precondition checks just as a sanity check
##########
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:
So there is actually no worry here about TableConfig inconsistency. The
TableConfig will have to be updated to disable the forward index. We added a
check in the TableConfigUtils which checks for the following:
```
/**
* Validates the compatibility of the indexes if the column has the
forward index disabled. Throws exceptions due to
* compatibility mismatch. The checks performed are:
* - Validate dictionary is enabled.
* - Validate inverted index is enabled.
* - Validate that either no range index exists for column or the
range index version is at least 2 and isn't a
* multi-value column (since multi-value defaults to index v1).
*/
private static void validateForwardIndexDisabledIndexCompatibility()
```
I will update this to add back my Precondition checks just as a sanity check
--
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]
