xiangfu0 commented on code in PR #19900:
URL: https://github.com/apache/pinot/pull/19900#discussion_r4289594932
##########
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:
Keeping the presence-only checks for single-column text/HNSW: they rebuild
before postUpdateIndicesCleanup removes disabled forward indexes, so retained
values remain valid sources. Multi-column text runs after that cleanup and
needs the extra config guard. Adding it here would reject safe recovery;
IndexReaderFactory also treats the physical segment reader as the source of
truth.
##########
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:
The compatible-writer path reads commit metadata only; full checksum/reader
validation is limited to newer writers. This adds metadata I/O per configured
index during loading, and we have not measured the cost at production segment
counts. Keeping that tradeoff explicit without claiming a workload-validated
overhead figure.
--
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]
