Jackie-Jiang commented on code in PR #19897:
URL: https://github.com/apache/pinot/pull/19897#discussion_r4340703505
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -264,21 +269,26 @@ private void buildMappingFile(File segmentIndexDir,
String column, Directory dir
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);
+ NumericDocValues docIds = MultiDocValues.getNumericValues(indexReader,
PINOT_DOC_ID_DOC_VALUES_FIELD_NAME);
Review Comment:
Non-blocking simplification: this loop and the reader's `fillDocIdMapping`
implement the same numeric lookup, stored-field fallback, validation, and
buffer writes. Could we share one helper in `TextIndexUtils`, taking an
`IndexReader`, buffer, document count, and optional `int[]`
mutable-to-immutable remap? Preserve the per-document fallback and apply any
remap before checking whether the mapping is an identity. This would also let
the creator use the visitor instead of constructing a `Document` and parsing a
string for each fallback document.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -58,6 +62,7 @@
public class LuceneTextIndexCreator extends AbstractTextIndexCreator {
private static final Logger LOGGER =
LoggerFactory.getLogger(LuceneTextIndexCreator.class);
public static final String LUCENE_INDEX_DOC_ID_COLUMN_NAME = "DocID";
+ public static final String PINOT_DOC_ID_DOC_VALUES_FIELD_NAME = "PinotDocID";
Review Comment:
Non-blocking naming consistency: both constants identify Pinot IDs stored as
Lucene fields, but the names mix `LUCENE_INDEX`/`PINOT` and `COLUMN`/`FIELD`.
If cleaning these up together, a clearer pair is
`PINOT_DOC_ID_STORED_FIELD_NAME` and `PINOT_DOC_ID_DOC_VALUES_FIELD_NAME`.
Preserve the existing public constant as an alias if renamed, and keep the two
persisted field names (`DocID` and `PinotDocID`) unchanged. This is optional;
the new constant's name is already descriptive.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -542,4 +480,53 @@ public void intField(FieldInfo fieldInfo, int value) {
e);
}
}
+
+ /// Reads the actual Pinot ids after Lucene merges and sorting. Doc values
avoid stored-field decompression;
+ /// indexes created by older writers, including individual older leaves,
retain the stored-field fallback.
+ @VisibleForTesting
+ static boolean fillDocIdMapping(IndexSearcher indexSearcher, PinotDataBuffer
buffer, int numDocs)
+ throws IOException {
+ NumericDocValues docIds =
MultiDocValues.getNumericValues(indexSearcher.getIndexReader(),
+ LuceneTextIndexCreator.PINOT_DOC_ID_DOC_VALUES_FIELD_NAME);
+ class DocIdVisitor extends DocumentStoredFieldVisitor {
Review Comment:
Non-blocking readability suggestion: move this method-local class to a
`private static` nested `PinotDocIdVisitor` and extend `StoredFieldVisitor`
directly. Its declaration interrupts the mapping loop, and it never uses the
`Document` maintained by `DocumentStoredFieldVisitor`. If the shared mapping
helper moves to a utility, put the visitor alongside it. Keep one visitor
instance per mapping operation and reset its ID before each stored-field read;
the mutable instance should not be shared statically.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -542,4 +480,53 @@ public void intField(FieldInfo fieldInfo, int value) {
e);
}
}
+
+ /// Reads the actual Pinot ids after Lucene merges and sorting. Doc values
avoid stored-field decompression;
+ /// indexes created by older writers, including individual older leaves,
retain the stored-field fallback.
+ @VisibleForTesting
+ static boolean fillDocIdMapping(IndexSearcher indexSearcher, PinotDataBuffer
buffer, int numDocs)
+ throws IOException {
+ NumericDocValues docIds =
MultiDocValues.getNumericValues(indexSearcher.getIndexReader(),
+ LuceneTextIndexCreator.PINOT_DOC_ID_DOC_VALUES_FIELD_NAME);
+ class DocIdVisitor extends DocumentStoredFieldVisitor {
+ int _pinotDocId;
+
+ @Override
+ public Status needsField(FieldInfo fieldInfo) {
+ return
LuceneTextIndexCreator.LUCENE_INDEX_DOC_ID_COLUMN_NAME.equals(fieldInfo.name) ?
Status.YES : Status.NO;
+ }
+
+ @Override
+ public void intField(FieldInfo fieldInfo, int value) {
+ _pinotDocId = value;
+ }
+
+ @Override
+ public void stringField(FieldInfo fieldInfo, String value) {
+ _pinotDocId = Integer.parseInt(value);
+ }
+ }
+ DocIdVisitor visitor = new DocIdVisitor();
+ StoredFields storedFields = null;
+ boolean allIdsAreEqual = true;
Review Comment:
Non-blocking naming: use `pinotDocIdValues` for `docIds` and `luceneDocId`
for the loop index in both mapping loops, so the Lucene lookup position is
distinct from the Pinot ID returned. `isIdentityMapping` also describes
`allIdsAreEqual` more precisely: each Lucene ID equals its corresponding Pinot
ID, rather than all IDs equaling each other. The boolean name is carried over
from the old code, so this is an optional cleanup.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -491,33 +460,7 @@ private DocIdTranslator
createDocIdTranslator(PinotDataBuffer docIdMappingBuffer
try {
if (config.getDocIdTranslatorMode() == DocIdTranslatorMode.TryOptimize) {
LOGGER.debug("Creating lucene to pinot doc id mapping.");
- boolean allIdsAreEqual = true;
-
- class DocIdVisitor extends DocumentStoredFieldVisitor {
- int _pinotDocId;
-
- @Override
- public Status needsField(FieldInfo fieldInfo) {
- // assume doc id is the only stored document
- assert
LuceneTextIndexCreator.LUCENE_INDEX_DOC_ID_COLUMN_NAME.equals(fieldInfo.name);
- return Status.YES;
- }
-
- @Override
- public void intField(FieldInfo fieldInfo, int value) {
- _pinotDocId = value;
- }
- }
-
- DocIdVisitor visitor = new DocIdVisitor();
- StoredFields storedFields = _indexSearcher.storedFields();
-
- for (int i = 0; i < numDocs; i++) {
- storedFields.document(i, visitor);
- int pinotDocId = visitor._pinotDocId;
- allIdsAreEqual &= (i == pinotDocId);
- buffer.putInt(i * Integer.BYTES, pinotDocId);
- }
+ boolean allIdsAreEqual = fillDocIdMapping(_indexSearcher, buffer,
numDocs);
Review Comment:
Non-blocking simplification: call `fillDocIdMapping` once before the
`TryOptimize` check, handle the identity case, then use one common
`DefaultDocIdTranslator` return. The same cleanup applies to
`prepareDocIdTranslator`. Keep the existing `Skip`/existing-mapping early exits
and the directory path's config updates.
--
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]
