Jackie-Jiang commented on a change in pull request #7286:
URL: https://github.com/apache/pinot/pull/7286#discussion_r698817955
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/V3DefaultColumnHandler.java
##########
@@ -47,39 +45,34 @@ protected boolean updateDefaultColumn(String column,
DefaultColumnAction action)
throws Exception {
LOGGER.info("Starting default column action: {} on column: {}", action,
column);
- // For V3 segment format, only support ADD action
- // For UPDATE and REMOVE action, throw exception to drop and re-download
the segment
- if (action.isUpdateAction()) {
- throw new V3UpdateIndexException(
- "Default value indices for column: " + column + " cannot be updated
for V3 format segment.");
+ // For UPDATE and REMOVE action, delete existing dictionary and forward
index, and remove column metadata
+ if (action.isUpdateAction() || action.isRemoveAction()) {
+ removeColumnIndices(column);
}
-
+ // For REMOVE action, no more to do. For ADD and UPDATE action, need to
create
Review comment:
(nit) Move this into the previous if branch
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/InvertedIndexHandler.java
##########
@@ -84,7 +92,7 @@ private void createInvertedIndexForColumn(ColumnMetadata
columnMetadata)
if (_segmentWriter.hasIndexFor(column, ColumnIndexType.INVERTED_INDEX)) {
Review comment:
Remove this check? Same for other handlers
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -327,7 +326,7 @@ public void testUploadSameSegments()
}
@Test
- public void testInvertedIndexTriggering()
+ public void testInvertedIndexTriggeringCleanIndicesOnReloading()
Review comment:
(nit) Suggest still keeping one test, and add/remove indices twice since
the logic is very similar
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexHandlerFactory.java
##########
@@ -0,0 +1,63 @@
+/**
+ * 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.File;
+import
org.apache.pinot.segment.local.segment.index.loader.bloomfilter.BloomFilterHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.H3IndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.InvertedIndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.JsonIndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.LuceneFSTIndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.RangeIndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.TextIndexHandler;
+import org.apache.pinot.segment.spi.index.metadata.SegmentMetadataImpl;
+import org.apache.pinot.segment.spi.store.ColumnIndexType;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+
+
+public class IndexHandlerFactory {
+ private IndexHandlerFactory() {
+ }
+
+ private static final IndexHandler DEFAULT_HANDLER = () -> {
+ // noop by default.
+ };
+
+ public static IndexHandler newHandler(ColumnIndexType type, File indexDir,
SegmentMetadataImpl segmentMetadata,
Review comment:
We usually name the method `getIndexHandler()` for factory.
##########
File path: pinot-segment-local/src/test/resources/data/newColumnsSchema3.json
##########
@@ -34,10 +34,6 @@
"name": "newBooleanSVDimension",
"defaultNullValue": "false"
},
- {
Review comment:
Why removing this field?
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexHandler.java
##########
@@ -0,0 +1,27 @@
+/**
+ * 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;
+
+public interface IndexHandler {
+ /**
+ * Add new indices and remove obsolete indices.
Review comment:
(nit)
```suggestion
* Adds new indices and removes obsolete indices.
```
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/JsonIndexHandler.java
##########
@@ -88,7 +95,7 @@ private void createJsonIndexForColumn(ColumnMetadata
columnMetadata)
if (_segmentWriter.hasIndexFor(columnName, ColumnIndexType.JSON_INDEX)) {
Review comment:
We should be able to remove this check?
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java
##########
@@ -57,55 +57,58 @@ private ImmutableSegmentLoader() {
private static final Logger LOGGER =
LoggerFactory.getLogger(ImmutableSegmentLoader.class);
/**
- * For tests only.
+ * Loads the segment with empty schema and IndexLoadingConfig. This method
is used to
+ * access the segment without modifying it, i.e. in read-only mode.
*/
public static ImmutableSegment load(File indexDir, ReadMode readMode)
throws Exception {
IndexLoadingConfig defaultIndexLoadingConfig = new IndexLoadingConfig();
defaultIndexLoadingConfig.setReadMode(readMode);
- return load(indexDir, defaultIndexLoadingConfig, null);
+ return load(indexDir, defaultIndexLoadingConfig, null, false);
}
/**
- * For tests only.
+ * Loads the segment with empty schema but a specified IndexLoadingConfig
(mostly empty).
Review comment:
Remove the `mostly empty`? We don't have this assumption for this method
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -35,7 +35,6 @@
import java.util.List;
Review comment:
We should be able to remove the segment version override in this test
and test the v3 default column removal
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -362,56 +361,99 @@ public void testInvertedIndexTriggering()
long tableSizeWithNewIndex = getTableSize(offlineTableName);
assertTrue(tableSizeWithNewIndex > tableSizeWithDefaultIndex);
- // Update table config to remove all inverted index.
+ // Update table config to remove the new inverted index, and
+ // reload table to clean the new inverted indices physically.
tableConfig = getOfflineTableConfig();
-
tableConfig.getIndexingConfig().setInvertedIndexColumns(Collections.emptyList());
+
tableConfig.getIndexingConfig().setInvertedIndexColumns(getInvertedIndexColumns());
updateTableConfig(tableConfig);
-
- // Reload table just disables those indices, not clean them physically.
reloadOfflineTable(offlineTableName);
TestUtils.waitForCondition(aVoid -> {
try {
JsonNode queryResponse2 = postQuery(TEST_UPDATED_INVERTED_INDEX_QUERY);
- // Total docs should not change during reload
+ // Total docs should not change during reload, but num entries scanned
+ // gets back to total number of documents as the index is removed.
assertEquals(queryResponse2.get("totalDocs").asLong(), numTotalDocs);
return queryResponse2.get("numEntriesScannedInFilter").asLong() ==
numTotalDocs;
} catch (Exception e) {
throw new RuntimeException(e);
}
- }, 600_000L, "Failed to disable indices");
- long tableSizeAfterReload = getTableSize(offlineTableName);
- assertEquals(tableSizeAfterReload, tableSizeWithNewIndex);
+ }, 600_000L, "Failed to cleanup obsolete index");
+ assertEquals(getTableSize(offlineTableName), tableSizeWithDefaultIndex);
+ }
+
+ @Test
+ public void testInvertedIndexTriggeringCleanIndicesWithForceDownload()
+ throws Exception {
+ String offlineTableName =
TableNameBuilder.OFFLINE.tableNameWithType(getTableName());
Review comment:
Let's extract the index addition part of these 2 tests into a common
helper method because they are the same
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexHandler.java
##########
@@ -0,0 +1,27 @@
+/**
+ * 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;
+
+public interface IndexHandler {
Review comment:
Add some javadoc
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexHandlerFactory.java
##########
@@ -0,0 +1,63 @@
+/**
+ * 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.File;
+import
org.apache.pinot.segment.local.segment.index.loader.bloomfilter.BloomFilterHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.H3IndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.InvertedIndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.JsonIndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.LuceneFSTIndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.RangeIndexHandler;
+import
org.apache.pinot.segment.local.segment.index.loader.invertedindex.TextIndexHandler;
+import org.apache.pinot.segment.spi.index.metadata.SegmentMetadataImpl;
+import org.apache.pinot.segment.spi.store.ColumnIndexType;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+
+
+public class IndexHandlerFactory {
+ private IndexHandlerFactory() {
+ }
+
+ private static final IndexHandler DEFAULT_HANDLER = () -> {
Review comment:
Rename it to `NO_OP_HANDLER`?
--
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
