raghavyadav01 commented on code in PR #18758:
URL: https://github.com/apache/pinot/pull/18758#discussion_r4528558949
##########
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:
Since segments convert gradually, queries with options will fail only on
dictionary based segments of the same table. Can we pass options through
getDictIds instead of rejecting them?
##########
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:
Could we decide raw vs dictionary forward index up front instead of building
the operator and then replacing it? Also the raw path does a dictionary indexOf
per row, which is a binary search on strings. Is this path common enough to
matter?
##########
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:
This runs on every segment load for every text column. Could we move the
build mode lookup into one TextIndexUtils helper shared with the reader, and
skip it when buildOnDictionary is off in both the config and the segment?
##########
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:
This relies on getUniqueValuesSet() matching the dictionary id order
exactly. Can we read the values from the dictionary itself, or at least check
the count against the dictionary length?
##########
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:
Can we avoid the TEXT_MATCH check in the generic filter operators? Would it
be simpler to let dictionary mode honor null handling like other dictionary
predicates and fix TextMatchFilterOperator in a separate change?
--
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]
