raghavyadav01 commented on code in PR #19897:
URL: https://github.com/apache/pinot/pull/19897#discussion_r4365658050
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -331,6 +313,17 @@ public void add(String[] documents, int length) {
}
}
+ private void addDocIdFields(Document document) {
+ int docId = _nextDocId++;
+ // Keep the stored field for older readers. Immutable indexes add doc
values to avoid decompressing every stored
+ // document on segment load and retain the actual Pinot id when Lucene
flushes or merges reorder documents.
+ document.add(new StoredField(PINOT_DOC_ID_STORED_FIELD_NAME, docId));
+ // A TEXT column with this name must keep the stored-field fallback to
avoid conflicting Lucene field schemas.
+ if (_commitOnClose &&
!_textColumn.equals(PINOT_DOC_ID_DOC_VALUES_FIELD_NAME)) {
Review Comment:
With storeInSegmentFile=true, the mapping is built once in close() and the
reader uses the persisted buffer from then on, so these doc values are never
read again but still add ~10% to the index (and to tiered/remote downloads).
Same for TryOptimize/Default after the first load. Can we skip the doc values
when storeInSegmentFile is true, or make them opt-in?
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/LuceneTextIndexCreatorTest.java:
##########
@@ -111,9 +118,84 @@ public void testIndexWriterReaderMatchWithOrClause()
// Use TextIndex reader to validate that reads work
try (LuceneTextIndexReader reader = new LuceneTextIndexReader("foo",
INDEX_DIR, 5, new HashMap<>())) {
int[] matchedDocIds = reader.getDocIds("retain|keep").toArray();
- Assert.assertEquals(matchedDocIds.length, 2);
- Assert.assertEquals(matchedDocIds[0], 1);
- Assert.assertEquals(matchedDocIds[1], 2);
+ assertEquals(matchedDocIds.length, 2);
+ assertEquals(matchedDocIds[0], 1);
+ assertEquals(matchedDocIds[1], 2);
+ }
+ }
+
+ @Test
+ public void testDocValuesRetainStoredIdsForSingleAndMultiValueDocuments()
Review Comment:
Could we add coverage for storeInSegmentFile=true? That includes the
combined .text file read via LuceneTextIndexBufferReader, and an older combined
file with no mapping buffer falling back to fillDocIdMapping.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/TextIndexUtils.java:
##########
@@ -89,6 +95,63 @@ public void onChange(Set<String> changedConfigs, Map<String,
String> clusterConf
}
}
+ /// Reads the actual Pinot ids after Lucene merges and sorting, optionally
remapping mutable ids during conversion.
+ /// Doc values avoid stored-field decompression. Mutable indexes contain
only the stored field; older immutable
+ /// indexes and documents without doc values also use the per-document
stored-field fallback.
+ /// Returns whether the final mapping is the identity mapping.
+ public static boolean fillDocIdMapping(IndexReader indexReader,
PinotDataBuffer buffer, int numDocs,
+ @Nullable int[] mutableToImmutableDocIdMap)
+ throws IOException {
+ NumericDocValues pinotDocIdValues =
+ MultiDocValues.getNumericValues(indexReader,
LuceneTextIndexCreator.PINOT_DOC_ID_DOC_VALUES_FIELD_NAME);
+ PinotDocIdVisitor visitor = new PinotDocIdVisitor();
+ StoredFields storedFields = null;
+ boolean isIdentityMapping = true;
+ for (int luceneDocId = 0; luceneDocId < numDocs; luceneDocId++) {
+ int pinotDocId;
+ if (pinotDocIdValues != null &&
pinotDocIdValues.advanceExact(luceneDocId)) {
+ pinotDocId = Math.toIntExact(pinotDocIdValues.longValue());
+ } else {
+ if (storedFields == null) {
+ storedFields = indexReader.storedFields();
+ }
+ visitor._pinotDocId = Integer.MIN_VALUE;
+ storedFields.document(luceneDocId, visitor);
+ pinotDocId = visitor._pinotDocId;
+ if (pinotDocId == Integer.MIN_VALUE) {
+ throw new IOException("Missing Pinot document id for Lucene
document: " + luceneDocId);
+ }
+ }
+ if (mutableToImmutableDocIdMap != null) {
+ // Conversion can reorder Pinot rows independently of Lucene's own
document order.
+ pinotDocId = mutableToImmutableDocIdMap[pinotDocId];
+ }
+ isIdentityMapping &= luceneDocId == pinotDocId;
+ buffer.putInt((long) luceneDocId * Integer.BYTES, pinotDocId);
+ }
+ return isIdentityMapping;
Review Comment:
close() ignores this return value when storeInSegmentFile is true, so an
identity mapping file still gets written, and the reader then always builds a
DefaultDocIdTranslator, even in TryOptimize. Since we have the flag here now,
could close() skip the file and persist Skip instead?
--
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]
