xiangfu0 commented on code in PR #19897:
URL: https://github.com/apache/pinot/pull/19897#discussion_r4365815040
##########
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:
Fixed in
[485c9696f9](https://github.com/apache/pinot/commit/485c9696f9cd7e99271726b9d36ffabceb3cdaa6).
The creator now omits numeric doc values for storeInSegmentFile=true, and for
explicit Skip mode. The default uncombined path retains the first-mapping
optimization and its documented storage tradeoff. Updated the description to
scope the benchmark and usage accordingly; no new configuration option.
##########
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:
Fixed in
[485c9696f9](https://github.com/apache/pinot/commit/485c9696f9cd7e99271726b9d36ffabceb3cdaa6).
The creator now consumes the shared helper's identity result after
mutable-to-immutable remapping. In explicit TryOptimize it closes the temporary
mapping buffer, deletes an identity mapping, and persists Skip before combining
files. Default/unspecified modes retain their mapping; a nonidentity
TryOptimize mapping is retained with Default. Added persisted-mode and
query-result coverage, including mappings made identity or nonidentity by
remapping.
##########
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:
Added in
[485c9696f9](https://github.com/apache/pinot/commit/485c9696f9cd7e99271726b9d36ffabceb3cdaa6):
real storeInSegmentFile=true combined indexes exercise persisted
mapping/config and LuceneTextIndexBufferReader queries across unspecified,
Default, TryOptimize, and Skip modes. A legacy stored-only combined file has no
mapping buffer and its source directory is removed; permuted-ID query results
verify the fallback mapping is rebuilt from the combined buffer. Targeted tests
and required checks passed.
--
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]
