xiangfu0 commented on code in PR #17998:
URL: https://github.com/apache/pinot/pull/17998#discussion_r3012869027


##########
pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/index/TextIndexConfigTest.java:
##########
@@ -122,6 +116,15 @@ public void withSomeData()
     assertEquals(config.getLuceneMaxBufferSizeMB(), 1024, "Unexpected 
luceneMaxBufferSize");
   }
 
+  @Test
+  public void withLegacyNativeFstTypeIgnored()

Review Comment:
   Done. Removed `withLegacyNativeFstTypeIgnored` from TextIndexConfigTest and 
the corresponding dead tests from FstIndexConfigTest.



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/FSTIndexHandler.java:
##########
@@ -105,8 +114,22 @@ public void updateIndices(SegmentDirectory.Writer 
segmentWriter)
     // Remove indices not set in table config any more
     String segmentName = _segmentDirectory.getSegmentMetadata().getName();
     Set<String> columnsToAddIdx = new HashSet<>(_columnsToAddIdx);
+    Set<String> legacyNativeColumns = 
getColumnsWithLegacyNativeFstIndex(segmentWriter);
+    for (String column : legacyNativeColumns) {
+      LOGGER.info("Removing legacy native FST index from segment: {}, column: 
{}", segmentName, column);
+      segmentWriter.removeIndex(column, StandardIndexes.fst());
+      if (columnsToAddIdx.remove(column)) {
+        ColumnMetadata columnMetadata = 
_segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
+        if (shouldCreateFSTIndex(columnMetadata)) {
+          createFSTIndexForColumn(segmentWriter, columnMetadata);
+        }
+      }
+    }
     Set<String> existingColumns = 
segmentWriter.toSegmentDirectory().getColumnsWithIndex(StandardIndexes.fst());
     for (String column : existingColumns) {
+      if (legacyNativeColumns.contains(column)) {
+        continue;
+      }

Review Comment:
   Good catch. Restructured: legacy indexes are removed first, then 
`existingColumns` is fetched after removal so they won't appear. The guard and 
`recreatedLegacyNativeColumns` tracking are no longer needed. Applied the same 
simplification to TextIndexHandler.



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