xiangfu0 commented on code in PR #19900:
URL: https://github.com/apache/pinot/pull/19900#discussion_r4300128979
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/MultiColumnTextIndexHandler.java:
##########
@@ -113,6 +120,66 @@ public boolean needUpdateIndices(SegmentDirectory.Reader
segmentReader) {
}
}
+ /// Verify all original source columns before removing an incompatible
shared text index.
+ public void validateRecoverySources(SegmentDirectory.Reader segmentReader)
+ throws IOException {
+ if (_textIndexConfig != null && (isBuildInProgress() || isIncompatible()))
{
+ for (String column : _textIndexConfig.getColumns()) {
+ if (!segmentReader.hasIndexFor(column, StandardIndexes.forward())
+ ||
_fieldIndexConfigs.get(column).getConfig(StandardIndexes.forward()).isDisabled())
{
Review Comment:
Fixed in 0d97da6d57: early validation checks columns already present in
segment metadata; immediately before rebuilding, the handler validates every
configured source after default-column creation. New v1/v3 rollback tests
verify the added default column is searchable and the next load reuses the
rebuilt index. All 117 focused rollback/loading tests passed.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -398,13 +409,14 @@ private boolean processMultiColTextIndex(File indexDir,
SegmentDirectory.Writer
LOGGER.info("Removing multi-column text index from segment: {}",
segmentName);
removeMultiColumnTextIndex(indexDir);
} else if (create) {
+ MultiColumnTextIndexHandler handler =
+ new MultiColumnTextIndexHandler(_segmentDirectory,
_indexLoadingConfig, newConfig);
+ handler.validateRecoverySources(segmentWriter);
Review Comment:
Addressed in 0d97da6d57: the handler now owns metadata, index-directory, and
mapping removal for both replacement and config removal. It writes the marker
before deletion and keeps it on failure; new v1/v3 tests verify interrupted
removal finishes on the next load. Early source validation remains before the
other preprocessing handlers.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/VectorIndexHandler.java:
##########
@@ -136,6 +143,84 @@ && hasCombinedFile(indexDir, column, desiredBackend)) {
return false;
}
+ /// Checks recovery sources before other handlers can reconstruct a missing
forward index. A multi-value forward
+ /// index regenerated from dictionary/inverted indexes does not retain
vector coordinate order or duplicates.
+ public void validateRecoverySources(SegmentDirectory.Reader segmentReader)
+ throws IOException {
+ Set<String> existingColumns =
segmentReader.toSegmentDirectory().getColumnsWithIndex(StandardIndexes.vector());
+ for (Map.Entry<String, VectorIndexConfig> entry :
_vectorConfigs.entrySet()) {
+ String column = entry.getKey();
+ if (entry.getValue().resolveBackendType() == VectorBackendType.HNSW
+ && (hasHnswRecoveryMarker(column) || existingColumns.contains(column)
+ && isIncompatibleHnswIndex(segmentReader, column))) {
Review Comment:
Addressed in 0d97da6d57: the early HNSW source validator now reuses
needsHnswRecovery after the backend check. Marker-first short-circuiting and
create-time reevaluation are preserved; all 31 HNSW/vector tests passed.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexRollbackRecoveryTest.java:
##########
@@ -0,0 +1,417 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.index.loader.invertedindex;
+
+import java.io.File;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipInputStream;
+import org.apache.commons.configuration2.PropertiesConfiguration;
+import org.apache.commons.io.FileUtils;
+import org.apache.lucene.analysis.standard.StandardAnalyzer;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StoredField;
+import org.apache.lucene.document.TextField;
+import org.apache.lucene.index.DirectoryReader;
+import org.apache.lucene.index.IndexWriter;
+import org.apache.lucene.index.IndexWriterConfig;
+import org.apache.lucene.store.FSDirectory;
+import org.apache.pinot.segment.local.PinotBuffersAfterClassCheckRule;
+import
org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader;
+import
org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl;
+import
org.apache.pinot.segment.local.segment.creator.impl.text.LuceneTextIndexCombined;
+import
org.apache.pinot.segment.local.segment.creator.impl.text.LuceneTextIndexCreator;
+import org.apache.pinot.segment.local.segment.index.loader.IndexLoadingConfig;
+import org.apache.pinot.segment.local.segment.index.loader.LoaderUtils;
+import
org.apache.pinot.segment.local.segment.index.readers.text.MultiColumnLuceneTextIndexReader;
+import org.apache.pinot.segment.local.segment.readers.GenericRowRecordReader;
+import org.apache.pinot.segment.local.segment.store.SegmentLocalFSDirectory;
+import org.apache.pinot.segment.local.utils.LuceneIndexCompatibilityUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
+import org.apache.pinot.segment.spi.creator.SegmentVersion;
+import org.apache.pinot.segment.spi.index.StandardIndexes;
+import
org.apache.pinot.segment.spi.index.multicolumntext.MultiColumnTextIndexConstants;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.segment.spi.store.SegmentDirectoryPaths;
+import org.apache.pinot.segment.spi.utils.SegmentMetadataUtils;
+import org.apache.pinot.spi.config.table.FieldConfig;
+import org.apache.pinot.spi.config.table.MultiColumnTextIndexConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.utils.ReadMode;
+import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertThrows;
+import static org.testng.Assert.assertTrue;
+
+
+/// Exercises server loading with real Lucene 10 indexes and retained Pinot
source values on Lucene 9.12.
+public class TextIndexRollbackRecoveryTest implements
PinotBuffersAfterClassCheckRule {
+ private File _tempDir;
+ private File _fixtures;
+
+ @BeforeMethod
+ public void setUp()
+ throws Exception {
+ _tempDir = Files.createTempDirectory("text-rollback").toFile();
+ _fixtures = new File(_tempDir, "fixtures");
+ try (InputStream resource =
getClass().getClassLoader().getResourceAsStream("data/lucene10-rollback.zip");
Review Comment:
Addressed in 0d97da6d57: all three tests now use
LuceneIndexTestUtils.extractLucene10Fixtures, which checks resource presence,
handles directory entries, and confines output paths. Each test still selects
its own fixture directory. The focused suite passed.
--
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]
