Jackie-Jiang commented on code in PR #19900:
URL: https://github.com/apache/pinot/pull/19900#discussion_r4300031226
##########
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:
Cleanup suggestion: after checking that the desired backend is HNSW, reuse
needsHnswRecovery(segmentReader, column) here instead of fetching
existingColumns and independently assembling the recovery condition.
isIncompatibleHnswIndex() already returns false when no HNSW representation
exists, and the helper preserves the marker-first short-circuit. This removes
the extra set lookup and keeps recovery detection consistent. Preserve the
early validation and create-time marker check, and reevaluate after mutations
rather than caching the result.
##########
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:
Cleanup suggestion: let MultiColumnTextIndexHandler own the complete
removal/replacement lifecycle. The preprocessor currently validates and deletes
the old metadata/files, then updateIndices() validates and deletes the index
files again. Moving this lifecycle into the handler would leave the
preprocessor responsible for checking whether work is needed, throttling, and
invocation, and would put marker creation before the first destructive
operation. Keep the early source validation before the other preprocessing
handlers; that check protects the original values and is still needed.
##########
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:
[P2] Allow newly added default columns through the early recovery
validation. If the incompatible shared index covers [a,b] and the updated
schema/config adds a new STRING column c, this rejects c because its forward
index does not exist yet. SegmentPreProcessor invokes this before
DefaultColumnHandler creates newly added columns, so loading fails even when
all original values for a and b are retained. Limit the early check to columns
already present in segment metadata, then validate all required sources
immediately before rebuilding. A regression case combining rollback with a
newly added default column would cover this.
##########
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:
Cleanup suggestion: share the lucene10-rollback.zip extraction in a small
test helper. This class, HnswIndexRollbackTest, and
LuceneIndexCompatibilityUtilsTest each unpack the same fixture with different
resource checks, directory-entry handling, and path handling. A shared
extractor returning the fixture root would remove duplication while letting
each test select its own fixture directory.
--
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]
