Jackie-Jiang commented on code in PR #9333:
URL: https://github.com/apache/pinot/pull/9333#discussion_r966414059
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -816,6 +835,7 @@ public static void
addColumnMetadataInfo(PropertiesConfiguration properties, Str
String.valueOf(columnIndexCreationInfo.getTotalNumberOfEntries()));
properties.setProperty(getKeyFor(column, IS_AUTO_GENERATED),
String.valueOf(columnIndexCreationInfo.isAutoGenerated()));
+ properties.setProperty(getKeyFor(column, FORWARD_INDEX_DISABLED),
String.valueOf(forwardIndexDisabled));
Review Comment:
Not sure if we need this in the segment metadata. We determine whether the
index exists by reading the index file (e.g. there is no metadata for
`HAS_INVERTED_INDEX`, `HAS_RANGE_INDEX` etc.)
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java:
##########
@@ -88,17 +90,25 @@ public ForwardIndexCreator
newForwardIndexCreator(IndexCreationContext.Forward c
context.getMaxRowLengthInBytes());
}
} else {
- if (context.getFieldSpec().isSingleValueField()) {
- if (context.isSorted()) {
- return new
SingleValueSortedForwardIndexCreator(context.getIndexDir(),
context.getFieldSpec().getName(),
- context.getCardinality());
+ if (context.forwardIndexDisabled()) {
+ if (context.getFieldSpec().isSingleValueField()) {
+ return new SingleValueNoOpForwardIndexCreator();
Review Comment:
Since the creator is no-op, no need to separate the SV and MV. Ideally we
should skip the adding value to the creator step, which has unnecessary
overhead, but this can be addressed in the future.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/datasource/DataSourceMetadata.java:
##########
@@ -70,6 +70,11 @@ default boolean isSingleValue() {
*/
int getMaxNumValuesPerMVEntry();
+ /**
+ * Returns whether forward index is disabled for this column or not, returns
'true' if disabled
+ */
+ boolean forwardIndexDisabled();
Review Comment:
I don't think we need this in the metadata. Instead, we can make
`DateSource.getForwardIndex()` nullable, and perform proper null checks when it
is used. This is consistent with how we handle other indexes.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -196,6 +204,15 @@ public void init(SegmentGeneratorConfig
segmentCreationSpec, SegmentIndexCreatio
Preconditions.checkState(dictEnabledColumn ||
!invertedIndexColumns.contains(columnName),
"Cannot create inverted index for raw index column: %s", columnName);
+ boolean forwardIndexDisabled =
forwardIndexDisabledColumns.contains(columnName);
+ if (forwardIndexDisabled) {
+ Preconditions.checkState(dictEnabledColumn &&
invertedIndexColumns.contains(columnName)
+ && !columnIndexCreationInfo.isSorted() &&
(!rangeIndexColumns.contains(columnName)
Review Comment:
We should allow sorted column
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/NoOpMVForwardIndexReader.java:
##########
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.index.readers.forward;
+
+import java.io.IOException;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReaderContext;
+import org.apache.pinot.spi.data.FieldSpec;
+
+
+/**
+ * No-op forward index reader meant to be used by multi-value columns with
forward index disabled. This is meant to
+ * provide clean exception handling for operations which need to read forward
index when forward index is disabled.
+ */
+public class NoOpMVForwardIndexReader implements
ForwardIndexReader<ForwardIndexReaderContext> {
Review Comment:
Suggest not using no-op reader as this is not consistent with how we handle
other index types. Instead, make `DataSource.getForwardIndex()` return `null`
when the forward index does not exist. When forward index is needed to solve
the query, check if the forward index is `null` and throw exception accordingly
--
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
