raghavyadav01 commented on code in PR #19897:
URL: https://github.com/apache/pinot/pull/19897#discussion_r4331942266
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -331,6 +341,16 @@ 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
Review Comment:
The reader persists the mapping file and reuses it on reload/restart, so the
faster build only kicks in on a segment's first load (or at creation with
`storeInSegmentFile`). Meanwhile every immutable index pays roughly 14 bits/doc
more on disk. Could you share index size and mapping build time before/after at
~1M and ~10M docs?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -331,6 +341,16 @@ 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(LUCENE_INDEX_DOC_ID_COLUMN_NAME, docId));
+ if (_commitOnClose) {
Review Comment:
Since this is gated on `_commitOnClose`, mutable indexes never get doc
values. So the `reuseMutableIndex` conversion always takes the stored-field
path, and the immutable index it produces has no doc values until it's rebuilt.
`RealtimeLuceneTextIndex.getPinotDocIds` also still reads a stored doc per hit
at query time. Was skipping mutable deliberate (NRT write cost), or worth
covering here too?
##########
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);
+ StoredFields storedFields = null;
+ 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.
Review Comment:
nit: "Older mutable indexes" reads as if new mutable indexes carry doc
values, but none do (see `addDocIdFields`). Maybe "Mutable indexes only contain
the stored field"?
##########
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)
Review Comment:
`MultiColumnLuceneTextIndexReader` and `MultiColumnLuceneTextIndexCreator`
still build the mapping from stored fields (with the old "DocID is the only
stored field" assert). Worth a follow-up to reuse this helper there?
--
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]
