xiangfu0 commented on code in PR #18758:
URL: https://github.com/apache/pinot/pull/18758#discussion_r4528990968
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/text/TextIndexType.java:
##########
@@ -103,8 +111,29 @@ protected ColumnConfigDeserializer<TextIndexConfig>
createDeserializerForLegacyC
@Override
public TextIndexCreator createIndexCreator(IndexCreationContext context,
TextIndexConfig indexConfig)
throws IOException {
-
Preconditions.checkState(context.getFieldSpec().getDataType().getStoredType()
== FieldSpec.DataType.STRING,
+
Preconditions.checkState(context.getFieldSpec().getDataType().getStoredType()
== DataType.STRING,
"Text index is currently only supported on STRING type columns");
+ if (indexConfig.isBuildOnDictionary()) {
+ Preconditions.checkState(context.getFieldSpec().isSingleValueField(),
+ "buildOnDictionary TEXT index requires a single-value column: %s",
context.getFieldSpec().getName());
+ Preconditions.checkState(context.hasDictionary(),
+ "buildOnDictionary TEXT index requires a dictionary-encoded column:
%s", context.getFieldSpec().getName());
+ LuceneTextIndexCreator creator = new LuceneTextIndexCreator(context,
indexConfig);
+ // Columnar segment-creation / realtime-conversion path: build one
Lucene document per distinct dictionary value
+ // (the stored id is the dictId). On the reload path ColumnStatistics is
null and TextIndexHandler feeds the
+ // dictionary values via add(String) instead.
+ ColumnStatistics columnStatistics = context.getColumnStatistics();
+ if (columnStatistics != null) {
+ Object uniqueValues = columnStatistics.getUniqueValuesSet();
Review Comment:
Added the cardinality check before opening the Lucene creator in
[7680774f71](https://github.com/apache/pinot/commit/7680774f713912c9f7cac323451a1150b3f935f1).
IndexCreationContext has no Dictionary handle; BaseSegmentCreator and the TEXT
creator consume the same sorted statistics values, so their array offsets match
dictionary IDs.
A new negative case rejects mismatched cardinality.
LuceneTextIndexCreatorTest (24 cases) and RealtimeSegmentConverterTest (227
cases, including normal and compacted dictionary conversion) pass on JDK 25.
All 575 focused tests, affected-module checks, and independent review pass.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/FilterOperatorUtils.java:
##########
@@ -86,21 +86,27 @@ public static boolean hasNulls(DataSource dataSource) {
return getNullBitmap(dataSource) != null;
}
+ static boolean isNullHandlingEnabled(QueryContext queryContext,
PredicateEvaluator predicateEvaluator) {
Review Comment:
Removed the TEXT_MATCH exception from the generic filters in
[7680774f71](https://github.com/apache/pinot/commit/7680774f713912c9f7cac323451a1150b3f935f1).
Dictionary-mode filters now honor ordinary query null handling;
TextMatchFilterOperator remains unchanged for a separate follow-up, as
requested.
Updated the null regression with explicit dictionary/per-row expectations
for positive, NOT, constant, mixed predicates, and selected row IDs.
DictionaryBasedTextIndexQueriesTest (12 cases) and
NullHandlingEnabledQueriesTest (159 cases) pass on JDK 25; all 575 focused
tests and affected-module checks pass.
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java:
##########
@@ -306,6 +308,26 @@ private BaseFilterOperator
constructPhysicalOperator(FilterContext filter, int n
Preconditions.checkState(textIndexReader != null,
"Cannot apply TEXT_MATCH on column: %s without text index",
column);
+ if (textIndexReader.isBuildOnDictionary()) {
+ // Dictionary-based text index: search returns matching
dictIds, resolved to docIds by the standard
+ // dictionary-based filter operators (inverted index / scan),
exactly like FST/REGEXP_LIKE.
+ TextMatchPredicate textMatchPredicate = (TextMatchPredicate)
predicate;
+ Preconditions.checkState(textMatchPredicate.getOptions() ==
null,
Review Comment:
Implemented in
[7680774f71](https://github.com/apache/pinot/commit/7680774f713912c9f7cac323451a1150b3f935f1):
a backward-compatible getDictIds(query, options) overload forwards options to
the existing Lucene query parser. Both filter and projection paths now pass
options through.
Mixed per-row/dictionary segment tests cover CLASSIC AND/OR, leading
wildcards, and MATCHPHRASE with directory and packed storage. All 575 focused
tests, affected-module checks, and independent review pass.
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java:
##########
@@ -306,6 +308,26 @@ private BaseFilterOperator
constructPhysicalOperator(FilterContext filter, int n
Preconditions.checkState(textIndexReader != null,
"Cannot apply TEXT_MATCH on column: %s without text index",
column);
+ if (textIndexReader.isBuildOnDictionary()) {
+ // Dictionary-based text index: search returns matching
dictIds, resolved to docIds by the standard
+ // dictionary-based filter operators (inverted index / scan),
exactly like FST/REGEXP_LIKE.
+ TextMatchPredicate textMatchPredicate = (TextMatchPredicate)
predicate;
+ Preconditions.checkState(textMatchPredicate.getOptions() ==
null,
+ "TEXT_MATCH options are not supported for a
dictionary-based text index on column: %s", column);
+ predicateEvaluator =
TextMatchDictIdPredicateEvaluatorFactory.newDictIdBasedEvaluator(
+ textMatchPredicate, textIndexReader,
dataSource.getDictionary());
+ BaseFilterOperator filterOperator =
+ FilterOperatorUtils.getLeafFilterOperator(_queryContext,
predicateEvaluator, dataSource, numDocs);
+ if (filterOperator instanceof ScanBasedFilterOperator &&
dataSource.getForwardIndex() != null
Review Comment:
Changed in
[7680774f71](https://github.com/apache/pinot/commit/7680774f713912c9f7cac323451a1150b3f935f1):
the factory chooses dictionary or raw evaluation before constructing the leaf,
using the existing per-query index eligibility helper. Constant matches retain
their shortcuts. Raw scans and projections precompute a matching-string set, so
there is no per-row dictionary.indexOf.
This fallback is limited to raw forward indexes with a retained dictionary
and no allowed dictionary-consuming index. Reload tests cover raw scans,
inverted filtering, and skipIndexes forcing a scan; counts agree and
scanned-entry checks confirm the selected path. All 575 focused tests pass on
JDK 25.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java:
##########
@@ -410,32 +421,43 @@ private void processMVField(SegmentDirectory.Writer
segmentWriter, boolean hasDi
}
}
- /// Checks if text index configuration has changed based on
storeInSegmentFile field config flag.
- ///
- /// The method uses TextIndexUtils.hasTextIndex() to determine the current
format and compare with desired format:
- /// - If TextIndexUtils.hasTextIndex() returns true: means text index in
directory format
- /// - If TextIndexUtils.hasTextIndex() returns false means no text index in
directory format
- ///
- /// @param columnName the column name to check
- /// @param segmentReader the segment reader to access the segment
- /// @return true if the text index configuration has changed and needs
reprocessing, false otherwise
- /// @throws Exception if there's an error checking the text index directory
structure
+ /// Checks whether the persisted text-index storage format or build mode
differs from the desired configuration.
private boolean hasTextIndexConfigurationChanged(String columnName,
SegmentDirectory.Reader segmentReader)
throws Exception {
- // Get current configuration
TextIndexConfig currentConfig =
_fieldIndexConfigs.get(columnName).getConfig(StandardIndexes.text());
-
- // Check if current config expects combined format
- boolean expectedFormat = currentConfig.isStoreInSegmentFile();
-
- // Check if existing index is in combined format using TextIndexUtils
File indexDir = _segmentDirectory.getSegmentMetadata().getIndexDir();
File segmentDirectory =
SegmentDirectoryPaths.segmentDirectoryFor(indexDir,
_segmentDirectory.getSegmentMetadata().getVersion());
boolean currentFormat = !TextIndexUtils.hasTextIndex(segmentDirectory,
columnName);
+ if (currentConfig.isStoreInSegmentFile() != currentFormat) {
+ return true;
+ }
- // If the expected format doesn't match the existing format, we need an
update
- return expectedFormat != currentFormat;
+ boolean buildOnDictionary = false;
Review Comment:
Centralized property extraction and persisted build-mode parsing in
TextIndexUtils for the handler and reader in
[7680774f71](https://github.com/apache/pinot/commit/7680774f713912c9f7cac323451a1150b3f935f1);
missing flags still mean per-row, and matching modes skip rebuilding.
Reload/mode-flip tests pass for both storage formats.
The initial persisted read is still necessary when the config is off:
segment metadata has no build-mode flag, so skipping that read would miss
dictionary-to-per-row rebuilds. This keeps downgrade/mode-flip safety without
adding format metadata or a stale global cache. All 575 focused tests and
affected-module checks pass.
--
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]
