xiangfu0 commented on code in PR #18758:
URL: https://github.com/apache/pinot/pull/18758#discussion_r3412037400
##########
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:
Good catch — fixed in 3c4ae9f3d3. `getUpdatedConfigFromPropertiesFile` now
defaults `buildOnDictionary` to `false` when the property is absent (instead of
falling back to the table-config value), so a per-row segment written before
this feature can never be misread as dictionary-based after the flag is enabled
at the table level. Added `segmentWithoutPersistedFlagReadsAsPerRow` which
strips the key from a per-row segment and reloads it under a
`buildOnDictionary=true` table config to verify it still reads as per-row.
##########
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:
Fixed in 3c4ae9f3d3 — the buffer-path `updateConfigFromProperties` now sets
`buildOnDictionary` to `false` when the property is missing rather than keeping
the table-config value, matching the file-based path. Same self-describe
guarantee, covered by the new regression test.
--
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]
