somandal commented on code in PR #9740:
URL: https://github.com/apache/pinot/pull/9740#discussion_r1015906038


##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -147,11 +190,45 @@ Map<String, Operation> 
computeOperation(SegmentDirectory.Reader segmentReader)
       }
     }
 
+    // Get list of columns with forward index and those without forward index
+    Set<String> existingForwardIndexColumns =
+        
segmentReader.toSegmentDirectory().getColumnsWithIndex(ColumnIndexType.FORWARD_INDEX);
+    Set<String> existingForwardIndexDisabledColumns = new HashSet<>();
+    for (String column : existingAllColumns) {
+      if (!existingForwardIndexColumns.contains(column)) {
+        existingForwardIndexDisabledColumns.add(column);
+      }
+    }
+
     // From new column config.
     Set<String> newNoDictColumns = 
_indexLoadingConfig.getNoDictionaryColumns();
+    Set<String> newForwardIndexDisabledColumns = 
_indexLoadingConfig.getForwardIndexDisabledColumns();
 
     for (String column : existingAllColumns) {
-      if (existingNoDictColumns.contains(column) && 
!newNoDictColumns.contains(column)) {
+      if (existingForwardIndexColumns.contains(column) && 
newForwardIndexDisabledColumns != null
+          && newForwardIndexDisabledColumns.contains(column)) {
+        // Existing column has a forward index. New column config disables the 
forward index
+        Preconditions.checkState(!newNoDictColumns.contains(column),
+            String.format("Must enable dictionary for disabling the forward 
index on column: %s", column));
+        
Preconditions.checkState(_indexLoadingConfig.getInvertedIndexColumns().contains(column),
+            String.format("Must enable inverted index for disabling the 
forward index on column: %s", column));

Review Comment:
   Actually there are checks in the TableConfig checker to ensure that this can 
never happen (these are checked on controller for any updates to table config). 
I don't actually need these checks here, but it was nice to be able to validate 
these in tests so just added them. I'll remove them altogether along for the 
tests that validate these conditions.



-- 
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]

Reply via email to