xiangfu0 commented on code in PR #19897:
URL: https://github.com/apache/pinot/pull/19897#discussion_r4364551396
##########
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:
Kept this deliberately scoped to newly built immutable indexes. Mutable/NRT
writers and per-hit query translation are unchanged; reuseMutableIndex copies
their stored-only representation and continues through the fallback. The
description now calls out this scope and that cached-mapping reloads bypass the
optimization. Extending NRT writes/queries would need its own ingestion/query
measurements.
##########
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:
Fixed in
https://github.com/apache/pinot/commit/5c9153975628af902279503127bd7d141f031460.
The shared helper documentation now says mutable indexes contain only the
stored field, with per-document fallback also covering old immutable indexes
and missing doc values.
##########
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:
Added 1M/10M index sizes, immutable-build samples, and mapping scan/fill
medians and ranges to the PR description, with a [reproducible harness and all
raw
results](https://gist.github.com/xiangfu0/d8340d75bd5b214a9c344e157e20c7ba).
The observed index cost was +9.30% / +11.51%; timings are explicitly labeled
warm-cache component observations on a contended host, with only one build
sample per variant/size. The scope now correctly says first mapping creation
(or creation-time work with storeInSegmentFile), while reloads with an existing
mapping bypass it.
--
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]
