xiangfu0 commented on code in PR #19897:
URL: https://github.com/apache/pinot/pull/19897#discussion_r4372968183
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -237,55 +241,47 @@ private void convertMutableSegment(File segmentIndexDir,
File consumerDir, @Null
indexWriter.commit();
buildMappingFile(segmentIndexDir, _textColumn, destDirectory,
mutableToImmutableDocIdMap);
} catch (Exception e) {
- throw new RuntimeException("Failed to build the mapping file during
segment conversion: " + e);
+ throw new RuntimeException("Failed to build the mapping file during
segment conversion", e);
}
} catch (IOException e) {
throw new RuntimeException("Failed to convert the mutable lucene index:
" + e);
}
}
- /// Generate the mapping file from mutable Pinot docId (stored within the
Lucene index) to immutable Pinot docId using
- /// the mutableToImmutableDocIdMap from segment conversion
+ /// Build a mapping from final Lucene ids to Pinot ids, applying
mutable-to-immutable remapping during conversion.
+ /// New immutable indexes persist either the mapping or a verified Skip
setting for older readers.
/// @param segmentIndexDir segment index directory
/// @param column column name
/// @param directory directory of the index
/// @param mutableToImmutableDocIdMap mutableToImmutableDocIdMap from
segment conversion
private void buildMappingFile(File segmentIndexDir, String column, Directory
directory,
@Nullable int[] mutableToImmutableDocIdMap)
throws IOException {
- IndexReader indexReader = DirectoryReader.open(directory);
- IndexSearcher indexSearcher = new IndexSearcher(indexReader);
-
- int numDocs = indexSearcher.getIndexReader().numDocs();
- int length = Integer.BYTES * numDocs;
- File docIdMappingFile = new
File(SegmentDirectoryPaths.findSegmentDirectory(segmentIndexDir),
- column +
V1Constants.Indexes.LUCENE_TEXT_INDEX_DOCID_MAPPING_FILE_EXTENSION);
- String desc = "Text index docId mapping buffer: " + column;
- try (PinotDataBuffer buffer = PinotDataBuffer.mapFile(docIdMappingFile, /*
readOnly */ false, 0, length,
- ByteOrder.LITTLE_ENDIAN, desc)) {
- try {
- // If immutableToMutableIdMap is null, then docIds should not change
between the mutable and immutable segments.
- // Therefore, the mapping file can be built without doing an
additional docId conversion
- if (mutableToImmutableDocIdMap == null) {
- for (int i = 0; i < numDocs; i++) {
- Document document = indexSearcher.storedFields().document(i);
- int pinotDocId =
Integer.parseInt(document.get(LuceneTextIndexCreator.LUCENE_INDEX_DOC_ID_COLUMN_NAME));
- buffer.putInt(i * Integer.BYTES, pinotDocId);
- }
- } else {
- for (int i = 0; i < numDocs; i++) {
- Document document = indexSearcher.storedFields().document(i);
- int mutableDocId =
Integer.parseInt(document.get(LuceneTextIndexCreator.LUCENE_INDEX_DOC_ID_COLUMN_NAME));
- int immutableDocId = mutableToImmutableDocIdMap[mutableDocId];
- buffer.putInt(i * Integer.BYTES, immutableDocId);
- }
- }
+ if (_config.getDocIdTranslatorMode() == DocIdTranslatorMode.Skip) {
+ return;
+ }
+ try (IndexReader indexReader = DirectoryReader.open(directory)) {
+ int numDocs = indexReader.numDocs();
+ int length = Integer.BYTES * numDocs;
+ File docIdMappingFile = new
File(SegmentDirectoryPaths.findSegmentDirectory(segmentIndexDir),
+ column +
V1Constants.Indexes.LUCENE_TEXT_INDEX_DOCID_MAPPING_FILE_EXTENSION);
+ String desc = "Text index docId mapping buffer: " + column;
+ boolean isIdentityMapping;
+ try (PinotDataBuffer buffer = PinotDataBuffer.mapFile(docIdMappingFile,
/* readOnly */ false, 0, length,
+ ByteOrder.LITTLE_ENDIAN, desc)) {
+ isIdentityMapping = TextIndexUtils.fillDocIdMapping(indexReader,
buffer, numDocs, mutableToImmutableDocIdMap);
} catch (Exception e) {
throw new RuntimeException(
"Caught exception while building mutable to immutable doc id
mapping for text index column: " + column, e);
}
- } finally {
- indexReader.close();
+ if (!_reuseMutableIndex || _config.getDocIdTranslatorMode() ==
DocIdTranslatorMode.TryOptimize) {
+ if (isIdentityMapping) {
+ FileUtils.delete(docIdMappingFile);
+ }
+ DocIdTranslatorMode mode = isIdentityMapping ?
DocIdTranslatorMode.Skip : DocIdTranslatorMode.Default;
+ TextIndexConfig newConfig = new
TextIndexConfigBuilder(_config).withDocIdTranslatorMode(mode.name()).build();
+ TextIndexUtils.writeConfigToPropertiesFile(_indexFile, newConfig);
+ }
Review Comment:
This persists `Skip` for a verified identity mapping in every non-reuse
mode, including an explicit `Default`. The enum still documents `Default` as
"build and keep mapping"
([TextIndexConfig](https://github.com/apache/pinot/blob/20b0442eb68dbeb4d807c78f5f459c7a1edef830/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/TextIndexConfig.java#L80-L86)),
and the reuse path still keeps the mapping under `Default`, so the two paths
now disagree. Results stay correct because every document is checked, but
either update the enum comment and docs to the new meaning, or keep the mapping
file when `Default` was chosen explicitly.
--
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]
