Jackie-Jiang commented on code in PR #9333:
URL: https://github.com/apache/pinot/pull/9333#discussion_r974673959
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -68,9 +68,14 @@ public DataFetcher(Map<String, DataSource> dataSourceMap) {
String column = entry.getKey();
DataSource dataSource = entry.getValue();
DataSourceMetadata dataSourceMetadata =
dataSource.getDataSourceMetadata();
+ ForwardIndexReader<?> forwardIndexReader = dataSource.getForwardIndex();
+ if (forwardIndexReader == null) {
+ throw new UnsupportedOperationException(
+ String.format("Forward index disabled for column: %s, cannot
create DataFetcher!",
+ dataSourceMetadata.getFieldSpec().getName()));
+ }
Review Comment:
(minor) Can be simplified to
```suggestion
Preconditions.checkState(forwardIndexReader != null, "Forward index
disabled for column: %s, cannot create DataFetcher!", column);
```
Note that we might want to throw `IllegalStateException` instead. Same for
other places
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java:
##########
@@ -133,7 +133,12 @@ public
PhysicalColumnIndexContainer(SegmentDirectory.Reader segmentReader, Colum
_rangeIndex = null;
}
- PinotDataBuffer fwdIndexBuffer = segmentReader.getIndexFor(columnName,
ColumnIndexType.FORWARD_INDEX);
+ // Setting the 'fwdIndexBuffer' to null if forward index is enabled. No-op
index readers will be setup for the
+ // forward index disabled columns which doesn't require the
'fwdIndexBuffer'.
+ boolean forwardIndexDisabled = !segmentReader.hasIndexFor(columnName,
ColumnIndexType.FORWARD_INDEX);
Review Comment:
(minor) `forwardIndexDisabled` is redundant. We can check if
`fwdIndexBuffer` is `null`
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java:
##########
@@ -58,6 +58,11 @@ public class TableConfig extends BaseJsonConfig {
// Double underscore is reserved for real-time segment name delimiter
private static final String TABLE_NAME_FORBIDDEN_SUBSTRING = "__";
+ // TODO: Remove this flag once the reload path to create forward index from
inverted index and dictionary is
+ // added. This feature will be disabled until the reload path is
updated to handle forward index enable ->
+ // disable and forward index disable -> enable.
+ public static boolean _disallowForwardIndexDisabled = true;
Review Comment:
IMO this is not needed. We can still enable this feature without ability to
automatically generate the forward index
##########
pinot-tools/src/main/java/org/apache/pinot/tools/segment/converter/DictionaryToRawIndexConverter.java:
##########
@@ -296,6 +297,10 @@ private void convertOneColumn(IndexSegment segment, String
column, File newSegme
throws IOException {
DataSource dataSource = segment.getDataSource(column);
ForwardIndexReader reader = dataSource.getForwardIndex();
+ if (reader == null) {
+ throw new UnsupportedEncodingException(String.format("Forward index is
disabled for column %s, cannot convert!",
Review Comment:
Why this exception?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -315,6 +341,19 @@ public List<String> getSortedColumns() {
return _sortedColumns;
}
+ /**
+ * For tests only.
+ */
+ @VisibleForTesting
+ public void setSortedColumns(String sortedColumn) {
Review Comment:
```suggestion
public void setSortedColumn(String sortedColumn) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -897,6 +903,41 @@ private static void validateFieldConfigList(@Nullable
List<FieldConfig> fieldCon
}
}
+ /**
+ * Validates the compatibility of the indexes if the column has the forward
index disabled. Throws exceptions due to
+ * compatibility mismatch. The checks performed are:
+ * - Validate dictionary is enabled.
+ * - Validate inverted index is enabled.
+ * - Validate that either no range index exists for column or the range
index version is at least 2 and isn't a
+ * multi-value column (since mulit-value defaults to index v1).
+ */
+ private static void validateForwardIndexDisabledIndexCompatibility(String
columnName, FieldConfig fieldConfig,
+ IndexingConfig indexingConfigs, List<String> noDictionaryColumns, Schema
schema) {
+ Map<String, String> fieldConfigProperties = fieldConfig.getProperties();
+ if (fieldConfigProperties == null) {
+ return;
+ }
+
+ boolean forwardIndexDisabled = Boolean.parseBoolean(fieldConfigProperties
+ .getOrDefault(FieldConfig.FORWARD_INDEX_DISABLED,
FieldConfig.DEFAULT_FORWARD_INDEX_DISABLED));
Review Comment:
(minor)
```suggestion
boolean forwardIndexDisabled =
Boolean.parseBoolean(fieldConfigProperties.get(FieldConfig.FORWARD_INDEX_DISABLED));
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/column/PhysicalColumnIndexContainer.java:
##########
@@ -149,7 +154,12 @@ public
PhysicalColumnIndexContainer(SegmentDirectory.Reader segmentReader, Colum
return;
}
}
- _forwardIndex =
indexReaderProvider.newForwardIndexReader(fwdIndexBuffer, metadata);
+ if (forwardIndexDisabled && !metadata.isSorted()) {
+ // Forward index disabled flag is a no-op for sorted columns
+ _forwardIndex = null;
+ } else {
+ _forwardIndex =
indexReaderProvider.newForwardIndexReader(fwdIndexBuffer, metadata);
+ }
Review Comment:
(minor) No need to check sorted here
```suggestion
if (fwdIndexBuffer != null) {
_forwardIndex =
indexReaderProvider.newForwardIndexReader(fwdIndexBuffer, metadata);
} else {
_forwardIndex = null;
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java:
##########
@@ -174,18 +175,46 @@ private void
createTextIndexForColumn(SegmentDirectory.Writer segmentWriter, Col
// segmentDirectory is indicated to us by SegmentDirectoryPaths, we create
lucene index there. There is no
// further need to move around the lucene index directory since it is
created with correct directory structure
// based on segmentVersion.
- try (ForwardIndexReader forwardIndexReader =
LoaderUtils.getForwardIndexReader(segmentWriter, columnMetadata);
- ForwardIndexReaderContext readerContext =
forwardIndexReader.createContext();
- TextIndexCreator textIndexCreator =
indexCreatorProvider.newTextIndexCreator(IndexCreationContext.builder()
-
.withColumnMetadata(columnMetadata).withIndexDir(segmentDirectory).build().forTextIndex(_fstType,
true))) {
- if (columnMetadata.isSingleValue()) {
- processSVField(segmentWriter, hasDictionary, forwardIndexReader,
readerContext, textIndexCreator, numDocs,
- columnMetadata);
+ try (TextIndexCreator textIndexCreator =
indexCreatorProvider.newTextIndexCreator(
+
IndexCreationContext.builder().withColumnMetadata(columnMetadata).withIndexDir(segmentDirectory).build()
+ .forTextIndex(_fstType, true))) {
+ boolean forwardIndexDisabled = !segmentWriter.hasIndexFor(columnName,
ColumnIndexType.FORWARD_INDEX);
+ if (forwardIndexDisabled) {
+ try (Dictionary dictionary = LoaderUtils.getDictionary(segmentWriter,
columnMetadata)) {
+ // Create the text index if the dictionary length is 1 as this is
for a default column (i.e. newly added
+ // column). For existing columns it is not possible to create the
text index without forward index
+ Preconditions.checkState(dictionary.length() == 1,
String.format("Creating text index for forward index "
Review Comment:
Suggest not adding this part but directly throwing exception. I don't see
the point of special handling generating text index for a column with all same
values. Same for other index handlers.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:
##########
@@ -348,6 +348,7 @@ public static class Builder {
private PartitionFunction _partitionFunction;
private Set<Integer> _partitions;
private boolean _autoGenerated;
+ private boolean _forwardIndexDisabled;
Review Comment:
Revert the changes in this file?
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
