xiangfu0 commented on code in PR #18758:
URL: https://github.com/apache/pinot/pull/18758#discussion_r4409518671
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/TextIndexUtils.java:
##########
@@ -530,6 +530,9 @@ public static void writeConfigToPropertiesFile(File
indexDir, TextIndexConfig co
properties.setProperty(FieldConfig.TEXT_INDEX_LUCENE_ANALYZER_CLASS_ARG_TYPES,
escapedLuceneAnalyzerClassArgTypes);
properties.setProperty(FieldConfig.TEXT_INDEX_LUCENE_QUERY_PARSER_CLASS,
config.getLuceneQueryParserClass());
properties.setProperty(FieldConfig.TEXT_INDEX_LUCENE_DOC_ID_TRANSLATOR_MODE,
config.getDocIdTranslatorMode());
+ // Persist the build mode so the reader self-describes (a table may hold a
mix of dictionary-based and per-row
+ // text segments; reads must not depend on the live table config).
+ properties.setProperty(FieldConfig.TEXT_INDEX_BUILD_ON_DICTIONARY,
config.isBuildOnDictionary());
Review Comment:
A segment built with `buildOnDictionary=true` is silently misread by
pre-feature servers during a rolling upgrade or downgrade: they ignore this
property, treat the index as per-row, and interpret the translated dictIds as
docIds, so `TEXT_MATCH` returns wrong rows with no error. Worth a bullet in the
Upgrade Notes/docs: enable the flag only after all servers run this version,
and don't downgrade while dictionary-based text segments exist.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/LuceneTextIndexCreator.java:
##########
@@ -153,7 +160,10 @@ public LuceneTextIndexCreator(String column, File
segmentIndexDir, boolean commi
// 1. Not the realtime index, i.e. commit is set to true
// 2. Happens during realtime segment conversion
// 3. Mutable segment not compacted
- _reuseMutableIndex = config.isReuseMutableIndex() && commit &&
realtimeConversion && !mutableSegmentCompacted;
+ // 4. Not a dictionary-based index: the mutable index is per-row, so a
dictionary-based immutable index must be
+ // rebuilt from the sealed dictionary rather than reusing the mutable
index.
+ _reuseMutableIndex = config.isReuseMutableIndex() && commit &&
realtimeConversion && !mutableSegmentCompacted
+ && !_buildOnDictionary;
Review Comment:
The realtime→offline conversion leg of this — `reuseMutableIndex` overridden
plus the creator being fed from `MutableColumnStatistics.getUniqueValuesSet()`
— has no test; `DictionaryBasedTextIndexQueriesTest` only covers offline build
and reload. A `RealtimeSegmentConverter`-style test with
`buildOnDictionary=true` and `reuseMutableIndex=true` asserting the sealed
segment self-describes as dictionary-based (ideally including the
upsert-compacted `CompactedColumnStatistics` variant) would close the gap.
--
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]
