Copilot commented on code in PR #17437:
URL: https://github.com/apache/pinot/pull/17437#discussion_r2651468291
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -407,6 +421,18 @@ public void close()
try {
// based on the commit flag set in IndexWriterConfig, this will decide
to commit or not
_indexWriter.close();
+ // Build docIdMapping file if storeInSegmentFile is true
+ // This allows the mapping file to be available during read without
building it on-the-fly
+ if (_config.isStoreInSegmentFile()) {
+ //Check if mapping file already exists
+ if (new
File(SegmentDirectoryPaths.findSegmentDirectory(_segmentDirectory),
+ _textColumn +
V1Constants.Indexes.LUCENE_TEXT_INDEX_DOCID_MAPPING_FILE_EXTENSION).exists()) {
+ return;
+ }
+ LOGGER.info("lucene doc IdMapping file doesn't exists for column: {},
building mapping file", _textColumn);
+ // Build the docId mapping file so it's available during segment load
+ buildMappingFile(_segmentDirectory, _textColumn, _indexDirectory,
null);
Review Comment:
The early return at line 430 bypasses the cleanup logic that follows
(closing `_indexDirectory` at line 436). This means if the mapping file already
exists, the index directory is never closed, potentially causing resource
leaks. Consider restructuring to ensure cleanup always happens, perhaps using a
flag or wrapping the mapping file creation in a conditional block without early
return.
```suggestion
// Check if mapping file already exists; build only if it does not
File mappingFile = new
File(SegmentDirectoryPaths.findSegmentDirectory(_segmentDirectory),
_textColumn +
V1Constants.Indexes.LUCENE_TEXT_INDEX_DOCID_MAPPING_FILE_EXTENSION);
if (!mappingFile.exists()) {
LOGGER.info("lucene doc IdMapping file doesn't exists for column:
{}, building mapping file", _textColumn);
// Build the docId mapping file so it's available during segment
load
buildMappingFile(_segmentDirectory, _textColumn, _indexDirectory,
null);
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -407,6 +421,18 @@ public void close()
try {
// based on the commit flag set in IndexWriterConfig, this will decide
to commit or not
_indexWriter.close();
+ // Build docIdMapping file if storeInSegmentFile is true
+ // This allows the mapping file to be available during read without
building it on-the-fly
+ if (_config.isStoreInSegmentFile()) {
+ //Check if mapping file already exists
Review Comment:
Missing space after `//` in the comment. Java code style typically requires
a space after the comment delimiter for readability.
```suggestion
// Check if mapping file already exists
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -407,6 +421,18 @@ public void close()
try {
// based on the commit flag set in IndexWriterConfig, this will decide
to commit or not
_indexWriter.close();
+ // Build docIdMapping file if storeInSegmentFile is true
+ // This allows the mapping file to be available during read without
building it on-the-fly
+ if (_config.isStoreInSegmentFile()) {
+ //Check if mapping file already exists
+ if (new
File(SegmentDirectoryPaths.findSegmentDirectory(_segmentDirectory),
+ _textColumn +
V1Constants.Indexes.LUCENE_TEXT_INDEX_DOCID_MAPPING_FILE_EXTENSION).exists()) {
+ return;
+ }
+ LOGGER.info("lucene doc IdMapping file doesn't exists for column: {},
building mapping file", _textColumn);
+ // Build the docId mapping file so it's available during segment load
Review Comment:
The `buildMappingFile` method call passes `null` as the fourth parameter
without explanation. Add a comment explaining what this parameter represents
and why `null` is appropriate here, to improve code maintainability.
```suggestion
// Build the docId mapping file so it's available during segment load
// The last argument is an optional reusable
IndexReader/IndexSearcher; null indicates that
// buildMappingFile should create and manage its own short-lived
reader for this offline build.
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -407,6 +421,18 @@ public void close()
try {
// based on the commit flag set in IndexWriterConfig, this will decide
to commit or not
_indexWriter.close();
+ // Build docIdMapping file if storeInSegmentFile is true
+ // This allows the mapping file to be available during read without
building it on-the-fly
+ if (_config.isStoreInSegmentFile()) {
+ //Check if mapping file already exists
+ if (new
File(SegmentDirectoryPaths.findSegmentDirectory(_segmentDirectory),
+ _textColumn +
V1Constants.Indexes.LUCENE_TEXT_INDEX_DOCID_MAPPING_FILE_EXTENSION).exists()) {
+ return;
+ }
+ LOGGER.info("lucene doc IdMapping file doesn't exists for column: {},
building mapping file", _textColumn);
Review Comment:
Corrected spelling of 'doesn't exists' to 'doesn't exist' and removed extra
space before 'building'.
```suggestion
LOGGER.info("lucene doc IdMapping file doesn't exist for column: {},
building mapping file", _textColumn);
```
--
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]
