Jackie-Jiang commented on code in PR #17998:
URL: https://github.com/apache/pinot/pull/17998#discussion_r3012146985
##########
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:
This seems redundant since legacy native columns are already removed from
columnsToAddIdx
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java:
##########
@@ -339,23 +362,13 @@ private boolean hasTextIndexConfigurationChanged(String
columnName, SegmentDirec
}
/**
- * Clean up existing text index files for recovery purposes.
- *
- * @param indexDir the index directory
- * @param columnName the column name
+ * Delete legacy native text index files from both the segment root and the
versioned segment directory.
*/
- private void cleanupExistingTextIndexFiles(File indexDir, String columnName)
{
- // Remove any existing text index files for this column
- File[] textIndexFiles = indexDir.listFiles((dir, name) -> {
- // Look for text index files for this column
- return name.startsWith(columnName) &&
(name.endsWith(V1Constants.Indexes.LUCENE_V912_TEXT_INDEX_FILE_EXTENSION)
- || name.endsWith(".text.inprogress"));
- });
-
- if (textIndexFiles != null) {
- for (File file : textIndexFiles) {
- FileUtils.deleteQuietly(file);
- }
+ private void deleteLegacyNativeTextIndexFiles(File indexDir, File
segmentDirectory, String column) {
Review Comment:
I think this is already handled within `segmentWriter.removeIndex(column,
StandardIndexes.text())`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java:
##########
@@ -140,6 +162,9 @@ public void updateIndices(SegmentDirectory.Writer
segmentWriter)
}
}
for (String column : existingColumns) {
+ if (recreatedLegacyNativeColumns.contains(column)) {
Review Comment:
This seems redundant since legacy native columns are already removed from
columnsToAddIdx
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java:
##########
@@ -339,23 +362,13 @@ private boolean hasTextIndexConfigurationChanged(String
columnName, SegmentDirec
}
/**
- * Clean up existing text index files for recovery purposes.
- *
- * @param indexDir the index directory
- * @param columnName the column name
+ * Delete legacy native text index files from both the segment root and the
versioned segment directory.
*/
- private void cleanupExistingTextIndexFiles(File indexDir, String columnName)
{
Review Comment:
Should we remove this call? This seems independent of this PR
--
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]
