xiangfu0 commented on code in PR #19900:
URL: https://github.com/apache/pinot/pull/19900#discussion_r4289535282
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/VectorIndexHandler.java:
##########
@@ -591,6 +685,9 @@ private void
createVectorIndexForColumn(SegmentDirectory.Writer segmentWriter, C
String columnName = columnMetadata.getColumnName();
VectorIndexConfig config =
_fieldIndexConfigs.get(columnName).getConfig(StandardIndexes.vector());
VectorBackendType backendType = config.resolveBackendType();
+ if (backendType == VectorBackendType.HNSW &&
hasHnswRecoveryMarker(columnName)) {
Review Comment:
Small consistency nit: the text handler switched its marker delete to
`FileUtils.forceDelete` so a silent delete failure can't leave a stale marker
that forces a rebuild on every future load. The vector path still uses
`FileUtils.deleteQuietly(inProgress)` at the end of
`createVectorIndexForColumn` (~line 737). Worth matching `forceDelete` here too
so a failed cleanup surfaces instead of looping. No data-loss risk, just wasted
work.
##########
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())
{
+ throw new IllegalStateException("Cannot rebuild multi-column Lucene
text index for column: " + column
+ + ". Original forward index must be retained; restore a
compatible segment or refresh it from source"
+ + " data");
+ }
+ }
+ }
+ }
+
+ private File getBuildMarker() {
+ File indexDir = _segmentDirectory.getSegmentMetadata().getIndexDir();
+ return indexDir == null
+ ? null
+ : new File(indexDir, MultiColumnTextIndexConstants.INDEX_DIR_NAME +
".text.inprogress");
+ }
+
+ private boolean isBuildInProgress() {
+ if (_textIndexConfig == null) {
+ return false;
+ }
+ File marker = getBuildMarker();
+ if (marker == null) {
+ return false;
+ }
+ // Older preprocessing removed metadata before building and wrote no
marker. A leftover shared directory
+ // without its metadata is also an unfinished build; it must never be
appended to on retry.
+ if (marker.exists()) {
+ return true;
+ }
+ return _segmentDirectory.getSegmentMetadata().getMultiColumnTextMetadata()
== null
+ &&
SegmentDirectoryPaths.findTextIndexIndexFile(_segmentDirectory.getSegmentMetadata().getIndexDir(),
+ MultiColumnTextIndexConstants.INDEX_DIR_NAME) != null;
+ }
+
+ private boolean isIncompatible() {
+ if (_segmentDirectory.getSegmentMetadata().getMultiColumnTextMetadata() ==
null || _textIndexConfig == null) {
+ return false;
+ }
+ File indexDir = _segmentDirectory.getSegmentMetadata().getIndexDir();
+ if (indexDir == null) {
+ return false;
+ }
+ File luceneDirectory =
+ SegmentDirectoryPaths.findTextIndexIndexFile(indexDir,
MultiColumnTextIndexConstants.INDEX_DIR_NAME);
+ if (luceneDirectory == null) {
+ return false;
+ }
+ try {
+ return LuceneIndexCompatibilityUtils.isIncompatible(luceneDirectory);
Review Comment:
Just flagging the cost for awareness: `isIncompatible()` opens the Lucene
commit header on every `needProcess` for any segment that has a text/HNSW
index, including healthy steady-state ones. It's header-only and short-circuits
when the index is absent, and it's needed for correctness, so this is fine —
but worth confirming it's acceptable at scale (many segments x many text/vector
columns).
##########
README.md:
##########
@@ -175,6 +175,21 @@ To verify which profile is active, run:
If you encounter issues, check that the `protoc-gen-grpc-java` binary is
present in one of the expected locations and is executable.
+### Lucene index recovery during loading
+
+Normal segment preprocessing automatically rebuilds text and HNSW indexes
whose Lucene format cannot be read by
+the running version. Single-column text, multi-column text, and
directory/packed HNSW indexes are rebuilt using
+the segment's original forward-index values and existing table configuration;
no recovery flag is needed.
+Compatible indexes are reused, and subsequent loads do not rebuild the
recovered indexes again. Interrupted builds
+restart from the retained source values before the segment is loaded.
+
+Install this recovery release on the intended rollback target before upgrading
Lucene. After a downgrade, loading
+segments containing newer Lucene indexes rebuilds those indexes with the older
writer. Retain the original forward
+indexes: dictionary/inverted-index reconstruction can lose multi-value
ordering and duplicate values. When original
+values are unavailable, recovery fails before replacing the incompatible
index; restore a compatible segment or
+refresh it from retained source data. Rebuilding adds CPU/I/O and
segment-loading time. Existing preprocessing
+opt-outs remain in effect, and corruption or ordinary I/O errors are not
treated as format incompatibility.
Review Comment:
Tiny doc nit: for the multi-column text path, a corrupt index actually makes
`needProcess`/loading throw (the IOException is wrapped as
IllegalStateException), so the segment fails to load rather than being quietly
reused. The sentence reads as if corrupt indexes are just left alone — a small
wording tweak would make the fail-fast behavior clearer to operators.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java:
##########
@@ -163,6 +171,108 @@ public void updateIndices(SegmentDirectory.Writer
segmentWriter)
}
}
+ /// Verify original source values before any preprocessing handler can
remove or regenerate them.
+ public void validateRecoverySources(SegmentDirectory.Reader segmentReader)
+ throws IOException {
+ for (String column : getExistingColumns(segmentReader)) {
+ if (_columnsToAddIdx.contains(column)
+ && (isBuildInProgress(column) || isIncompatible(column,
segmentReader))) {
+ validateRecoverySource(segmentReader, column);
+ }
+ }
+ }
+
+ private Set<String> getExistingColumns(SegmentDirectory.Reader
segmentReader) {
+ Set<String> columns =
+ new
HashSet<>(segmentReader.toSegmentDirectory().getColumnsWithIndex(StandardIndexes.text()));
+ File indexDir = _segmentDirectory.getSegmentMetadata().getIndexDir();
+ if (indexDir != null) {
+ File segmentDirectory =
+ SegmentDirectoryPaths.segmentDirectoryFor(indexDir,
_segmentDirectory.getSegmentMetadata().getVersion());
+ for (String column :
_segmentDirectory.getSegmentMetadata().getAllColumns()) {
+ File combined = new File(segmentDirectory,
+ column +
V1Constants.Indexes.LUCENE_COMBINE_TEXT_INDEX_FILE_EXTENSION);
+ if (combined.exists() || isBuildInProgress(column)) {
+ columns.add(column);
+ }
+ }
+ }
+ return columns;
+ }
+
+ private void validateRecoverySource(SegmentDirectory.Reader segmentReader,
String column) {
+ if (!segmentReader.hasIndexFor(column, StandardIndexes.forward())) {
+ throw new IllegalStateException("Cannot rebuild Lucene text index for
column: " + column
+ + ". Original forward index must be retained; restore a compatible
segment or refresh it from source data");
Review Comment:
Minor consistency point:
`MultiColumnTextIndexHandler.validateRecoverySources` also rejects when the new
config has the forward index disabled, but this single-column check (and the
HNSW one) only verify presence. It's safe today because disabled forward
indexes are deleted in `postUpdateIndicesCleanup`, after the rebuild has
already read them, but adding the same `isDisabled()` guard would keep the
three validators aligned and intent-explicit.
--
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]
