raghavyadav01 commented on code in PR #18758:
URL: https://github.com/apache/pinot/pull/18758#discussion_r4319339702
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -101,7 +106,11 @@ public LuceneTextIndexReader(String column, File indexDir,
int numDocs, TextInde
config =
TextIndexUtils.getUpdatedConfigFromPropertiesFile(propertiesFile, config);
}
- _docIdTranslator = prepareDocIdTranslator(indexDir, _column, numDocs,
_indexSearcher, config, indexFile);
+ _buildOnDictionary = config.isBuildOnDictionary();
+ // In dictionary mode the Lucene docId equals the dictId (entries are
added in dictId order and the index is
+ // force-merged to a single segment), so no docId mapping is needed.
+ _docIdTranslator = _buildOnDictionary ? NoOpDocIdTranslator.INSTANCE
Review Comment:
The no-op translator assumes Lucene docId == dictId, but that only holds if
Lucene keeps insertion order. The creator still uses the default merge policy,
and the comment in `LuceneTextIndexCreator` notes even LogByteSize doesn't
guarantee order. Once the dictionary is big enough to flush more than one
Lucene segment before `seal()`, the merge can reorder docs and TEXT_MATCH would
return the wrong dictIds (the tests use 1000 rows, so they never flush twice).
Can we keep the docId mapping here, since the stored doc id is already written,
or force a merge policy that only merges adjacent segments?
--
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]
