Copilot commented on code in PR #18758:
URL: https://github.com/apache/pinot/pull/18758#discussion_r3411918607


##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -331,6 +356,11 @@ private TextIndexConfig 
updateConfigFromProperties(Properties properties, TextIn
       builder.withDocIdTranslatorMode(docIdTranslatorMode);
     }
 
+    String buildOnDictionary = 
properties.getProperty(FieldConfig.TEXT_INDEX_BUILD_ON_DICTIONARY);
+    if (buildOnDictionary != null) {
+      builder.withBuildOnDictionary(Boolean.parseBoolean(buildOnDictionary));
+    }

Review Comment:
   For buffer-loaded indexes, if `buildOnDictionary` is absent from the 
persisted properties (segments created before this feature), the reader 
currently keeps the table-config value. That can incorrectly treat an old 
per-row index as dictionary-based. Prefer defaulting to `false` when the 
property is missing so the segment self-describes safely.



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/TextIndexUtils.java:
##########
@@ -437,6 +440,9 @@ public static TextIndexConfig 
getUpdatedConfigFromPropertiesFile(File file, Text
         .withLuceneAnalyzerClassArgTypes(recoveredLuceneAnalyzerClassArgTypes)
         
.withLuceneQueryParserClass(properties.getString(FieldConfig.TEXT_INDEX_LUCENE_QUERY_PARSER_CLASS))
         
.withDocIdTranslatorMode(properties.getString(FieldConfig.TEXT_INDEX_LUCENE_DOC_ID_TRANSLATOR_MODE))
+        // Default to the existing config value when the property is absent 
(segments written before this feature).
+        
.withBuildOnDictionary(properties.getBoolean(FieldConfig.TEXT_INDEX_BUILD_ON_DICTIONARY,
+            config.isBuildOnDictionary()))

Review Comment:
   When the segment properties file doesn’t contain `buildOnDictionary` (e.g., 
segments written before this feature), defaulting to the *table config* value 
can cause the reader to misinterpret a per-row index as dictionary-based (or 
vice versa), breaking the “segment self-describes” guarantee and potentially 
returning incorrect results.



-- 
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]

Reply via email to