Jackie-Jiang commented on code in PR #17998:
URL: https://github.com/apache/pinot/pull/17998#discussion_r3005890576
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java:
##########
@@ -339,23 +362,16 @@ private boolean hasTextIndexConfigurationChanged(String
columnName, SegmentDirec
}
/**
- * Clean up existing text index files for recovery purposes.
+ * Clean up text index files for both the segment root and the versioned
segment directory.
*
* @param indexDir the index directory
+ * @param segmentDirectory the versioned segment directory
* @param columnName the column name
*/
- 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 cleanupTextIndexFiles(File indexDir, File segmentDirectory,
String columnName) {
Review Comment:
I don't follow this change.
In order to remove text index file, you can use
`segmentWriter.removeIndex(column, StandardIndexes.text())`
Is `cleanupExistingTextIndexFiles()` no longer needed?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/store/SegmentDirectoryPaths.java:
##########
@@ -100,14 +100,15 @@ public static File findTextIndexIndexFile(File indexDir,
String column) {
}
/**
- * Find native text index file in top-level segment index directory
+ * Find native text index file in top-level segment index directory.
* @param indexDir top-level segment index directory
* @param column text column name
* @return text index directory (if existst), null if index file does not
exit
*/
@Nullable
+ @Deprecated
public static File findNativeTextIndexIndexFile(File indexDir, String
column) {
Review Comment:
Is this used?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/FSTIndexHandler.java:
##########
@@ -105,8 +114,23 @@ 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.contains(column)) {
+ ColumnMetadata columnMetadata =
_segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
+ if (shouldCreateFSTIndex(columnMetadata)) {
+ createFSTIndexForColumn(segmentWriter, columnMetadata);
+ }
+ columnsToAddIdx.remove(column);
Review Comment:
Combine this into the if 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]
