xiangfu0 commented on code in PR #19897:
URL: https://github.com/apache/pinot/pull/19897#discussion_r4288752312
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -264,21 +268,19 @@ 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++) {
+ NumericDocValues docIds = MultiDocValues.getNumericValues(indexReader,
PINOT_DOC_ID_DOC_VALUES_FIELD_NAME);
+ for (int i = 0; i < numDocs; i++) {
+ int pinotDocId;
+ if (docIds != null && docIds.advanceExact(i)) {
+ pinotDocId = Math.toIntExact(docIds.longValue());
+ } else {
+ // Older mutable indexes contain only the stored field.
Mixed-version leaves can also lack doc values.
Document document = indexSearcher.storedFields().document(i);
Review Comment:
Minor: this calls `indexSearcher.storedFields()` once per fallback document
inside the loop. For a DirectoryReader that allocates a fresh merged
stored-fields reader each time. The reader side (`fillDocIdMapping`) already
hoists this call and caches it, so it would be nice to do the same here for
consistency and to avoid the repeated allocation on older/mixed indexes.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -264,21 +268,19 @@ 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++) {
+ NumericDocValues docIds = MultiDocValues.getNumericValues(indexReader,
PINOT_DOC_ID_DOC_VALUES_FIELD_NAME);
+ for (int i = 0; i < numDocs; i++) {
+ int pinotDocId;
+ if (docIds != null && docIds.advanceExact(i)) {
+ pinotDocId = Math.toIntExact(docIds.longValue());
+ } else {
+ // Older mutable indexes contain only the stored field.
Mixed-version leaves can also lack doc values.
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);
+ pinotDocId =
Integer.parseInt(document.get(LUCENE_INDEX_DOC_ID_COLUMN_NAME));
Review Comment:
Minor: if the stored field were ever missing, `document.get(...)` returns
null and `Integer.parseInt(null)` throws NumberFormatException. The reader's
`fillDocIdMapping` guards this explicitly and throws a clear "Missing Pinot
document id" message. Mirroring that here would make the two paths behave
consistently on a corrupt index.
--
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]
