somandal commented on code in PR #9810:
URL: https://github.com/apache/pinot/pull/9810#discussion_r1039989286
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -72,34 +75,34 @@
* 1. Change compression on raw SV and MV columns.
* 2. Enabling dictionary on a raw column.
* 3. Disable forward index on a column where it is enabled.
+ * 4. Enable forward index on a forward index disabled column
*
* TODO: Add support for the following:
* 1. Disable dictionary
* 2. Segment versions < V3
- * 3. Enable forward index on a forward index disabled column
*/
-public class ForwardIndexHandler implements IndexHandler {
+public class ForwardIndexHandler extends BaseForwardIndexBasedIndexHandler {
private static final Logger LOGGER =
LoggerFactory.getLogger(ForwardIndexHandler.class);
- private final SegmentMetadata _segmentMetadata;
- private final IndexLoadingConfig _indexLoadingConfig;
+ // This should contain a list of all indexes that need to be rewritten if
the dictionary is enabled or disabled
+ private static final List<ColumnIndexType>
DICTIONARY_RELATED_INDEX_TO_REMOVE =
Review Comment:
done - added INVERTED + FST to this list
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/BaseForwardIndexBasedIndexHandler.java:
##########
@@ -0,0 +1,86 @@
+/**
+ * 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.loader;
+
+import java.io.IOException;
+import java.util.HashSet;
+import java.util.Set;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.SegmentMetadata;
+import org.apache.pinot.segment.spi.creator.IndexCreatorProvider;
+import org.apache.pinot.segment.spi.store.ColumnIndexType;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Base class for all of the {@link IndexHandler} classes which need to build
their indexes from the forward index.
+ * This class handles temporarily rebuilding the forward index if the forward
index does not exist and cleaning up
+ * the forward index once all handlers have completed via overriding the
postUpdateIndicesCleanup() method.
+ */
+public abstract class BaseForwardIndexBasedIndexHandler implements
IndexHandler {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(BaseForwardIndexBasedIndexHandler.class);
+
+ protected final SegmentMetadata _segmentMetadata;
+ protected final IndexLoadingConfig _indexLoadingConfig;
+ protected final Set<String> _forwardIndexDisabledColumnsToCleanup;
+
+ public BaseForwardIndexBasedIndexHandler(SegmentMetadata segmentMetadata,
IndexLoadingConfig indexLoadingConfig) {
+ _segmentMetadata = segmentMetadata;
+ _indexLoadingConfig = indexLoadingConfig;
+ _forwardIndexDisabledColumnsToCleanup = new HashSet<>();
+ }
+
+ @Override
+ public void postUpdateIndicesCleanup(SegmentDirectory.Writer segmentWriter)
+ throws Exception {
+ // Delete the forward index for columns which have it disabled. Perform
this as a post-processing step after all
+ // IndexHandlers have updated their indexes as some of them need to
temporarily create a forward index to
+ // generate other indexes off of.
+ for (String column : _forwardIndexDisabledColumnsToCleanup) {
+ segmentWriter.removeIndex(column, ColumnIndexType.FORWARD_INDEX);
+ }
+ }
+
+ protected void createForwardIndexIfNeeded(SegmentDirectory.Writer
segmentWriter, ColumnMetadata columnMetadata,
+ IndexCreatorProvider indexCreatorProvider, boolean
isTemporaryForwardIndex)
+ throws IOException {
+ String columnName = columnMetadata.getColumnName();
+ if (segmentWriter.hasIndexFor(columnName, ColumnIndexType.FORWARD_INDEX)) {
+ LOGGER.info("Forward index already exists for column: {}, skip trying to
create it", columnName);
+ return;
+ }
+
+ InvertedIndexAndDictionaryBasedForwardIndexCreator
invertedIndexAndDictionaryBasedForwardIndexCreator =
+ new InvertedIndexAndDictionaryBasedForwardIndexCreator(columnName,
_segmentMetadata, _indexLoadingConfig,
+ segmentWriter, indexCreatorProvider, isTemporaryForwardIndex);
+
invertedIndexAndDictionaryBasedForwardIndexCreator.constructForwardIndexFromInvertedIndexAndDictionary();
+ // If forward index is disabled it means that it has to be dictionary
based and the inverted index must exist.
Review Comment:
good catch, updated
--
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
