Jackie-Jiang commented on code in PR #10905:
URL: https://github.com/apache/pinot/pull/10905#discussion_r1244331242
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java:
##########
@@ -83,7 +86,11 @@ public MultipleTreesBuilder(List<StarTreeV2BuilderConfig>
builderConfigs, File i
_segmentDirectory = SegmentDirectoryPaths.findSegmentDirectory(indexDir);
_metadataProperties =
CommonsConfigurationUtils.fromFile(new File(_segmentDirectory,
V1Constants.MetadataKeys.METADATA_FILE_NAME));
-
Preconditions.checkState(!_metadataProperties.containsKey(MetadataKey.STAR_TREE_COUNT),
"Star-tree already exists");
+ _separator = getSeparator();
+ if (_separator != null) {
+ StarTreeBuilderUtils.removeStarTrees(indexDir);
+ _metadataProperties.reload();
Review Comment:
Why do we need to reload the metadata?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java:
##########
@@ -104,7 +111,11 @@ public MultipleTreesBuilder(@Nullable
List<StarTreeIndexConfig> indexConfigs, bo
_segmentDirectory = SegmentDirectoryPaths.findSegmentDirectory(indexDir);
_metadataProperties =
CommonsConfigurationUtils.fromFile(new File(_segmentDirectory,
V1Constants.MetadataKeys.METADATA_FILE_NAME));
-
Preconditions.checkState(!_metadataProperties.containsKey(MetadataKey.STAR_TREE_COUNT),
"Star-tree already exists");
+ _separator = getSeparator();
+ if (_separator != null) {
+ StarTreeBuilderUtils.removeStarTrees(indexDir);
+ _metadataProperties.refresh();
+ }
Review Comment:
We don't need this handling because this constructor is always used by the
segment creator, at which moment the star-tree will never exist.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/StarTreeIndexSeparator.java:
##########
@@ -0,0 +1,154 @@
+/**
+ * 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.startree.v2.builder;
+
+import com.google.common.collect.Lists;
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.RandomAccessFile;
+import java.nio.channels.FileChannel;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.configuration.Configuration;
+import org.apache.commons.configuration.PropertiesConfiguration;
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.segment.local.startree.v2.store.StarTreeIndexMapUtils;
+import org.apache.pinot.segment.spi.V1Constants;
+import
org.apache.pinot.segment.spi.index.startree.AggregationFunctionColumnPair;
+import org.apache.pinot.segment.spi.index.startree.StarTreeV2Constants;
+import org.apache.pinot.spi.config.table.StarTreeIndexConfig;
+
+
+/**
+ * The {@code StarTreeIndexSeparator} pulls out the individual star-trees from
the common star-tree index file
+ */
+public class StarTreeIndexSeparator implements Closeable {
+
+ private final FileChannel _indexFileChannel;
+ private final List<Map<StarTreeIndexMapUtils.IndexKey,
StarTreeIndexMapUtils.IndexValue>> _indexMapList;
+ private final List<StarTreeV2BuilderConfig> _builderConfigList;
+ private final List<Integer> _totalDocsList;
+
+ public StarTreeIndexSeparator(File indexMapFile, File indexFile,
PropertiesConfiguration metadataProperties)
+ throws IOException {
+ _indexMapList = extractIndexMap(indexMapFile,
+
metadataProperties.getInt(StarTreeV2Constants.MetadataKey.STAR_TREE_COUNT));
+ _indexFileChannel = new RandomAccessFile(indexFile, "r").getChannel();
+ _builderConfigList = extractBuilderConfigs(metadataProperties);
+ _totalDocsList = extractTotalDocsList(metadataProperties);
+ }
+
+ private List<Map<StarTreeIndexMapUtils.IndexKey,
StarTreeIndexMapUtils.IndexValue>> extractIndexMap(File indexMapFile,
+ int numStarTrees) {
+ try (InputStream inputStream = new FileInputStream(indexMapFile)) {
+ return StarTreeIndexMapUtils.loadFromInputStream(inputStream,
numStarTrees);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public List<Integer> extractTotalDocsList(PropertiesConfiguration
metadataProperties) {
+ List<Integer> totalDocsList = new ArrayList<>(_indexMapList.size());
+ for (int i = 0; i < _indexMapList.size(); i++) {
+ Configuration metadata =
metadataProperties.subset(StarTreeV2Constants.MetadataKey.getStarTreePrefix(i));
+ totalDocsList.add(i,
metadata.getInt(StarTreeV2Constants.MetadataKey.TOTAL_DOCS));
+ }
+ return totalDocsList;
+ }
+
+ /**
+ * Extract the list of {@link StarTreeV2BuilderConfig} for each of the
star-tree present in the given metadata
+ * properties.
+ * @param metadataProperties index metadata properties
+ * @return List of {@link StarTreeV2BuilderConfig}
+ */
+ public List<StarTreeV2BuilderConfig>
extractBuilderConfigs(PropertiesConfiguration metadataProperties) {
+ List<StarTreeV2BuilderConfig> builderConfigList = new
ArrayList<>(_indexMapList.size());
+ for (int i = 0; i < _indexMapList.size(); i++) {
+ Configuration metadata =
metadataProperties.subset(StarTreeV2Constants.MetadataKey.getStarTreePrefix(i));
+ builderConfigList.add(i, StarTreeV2BuilderConfig.fromIndexConfig(new
StarTreeIndexConfig(
+
Lists.newArrayList(metadata.getStringArray(StarTreeV2Constants.MetadataKey.DIMENSIONS_SPLIT_ORDER)),
+ Lists.newArrayList(
+
metadata.getStringArray(StarTreeV2Constants.MetadataKey.SKIP_STAR_NODE_CREATION_FOR_DIMENSIONS)),
+
Lists.newArrayList(metadata.getStringArray(StarTreeV2Constants.MetadataKey.FUNCTION_COLUMN_PAIRS)),
+ metadata.getInt(StarTreeV2Constants.MetadataKey.MAX_LEAF_RECORDS))));
+ }
+ return builderConfigList;
+ }
+
+ /**
+ * Extract star-tree index files of the star-tree represented by the
builderConfig.
+ * The extracted star-tree index files are written to the provided
starTreeOutputDir.
+ *
+ * @param starTreeOutputDir output directory for extracted star-trees
+ * @param builderConfig {@link StarTreeV2BuilderConfig} of the star-tree to
separate
+ * @return if star-tree exist then total docs in the separated tree, else -1
+ * @throws IOException
+ */
+ public int separate(File starTreeOutputDir, StarTreeV2BuilderConfig
builderConfig)
+ throws IOException {
+ int treeIndex = _builderConfigList.indexOf(builderConfig);
+ if (treeIndex == -1) {
+ return treeIndex;
Review Comment:
(minor) this should be `return -1` (although the same effect, but logically
it should return the docs)
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java:
##########
@@ -115,13 +126,38 @@ public MultipleTreesBuilder(@Nullable
List<StarTreeIndexConfig> indexConfigs, bo
}
}
+ private StarTreeIndexSeparator getSeparator()
+ throws Exception {
+ if (!_metadataProperties.containsKey(MetadataKey.STAR_TREE_COUNT)) {
+ return null;
+ }
+ try {
+ _separatorTempDir = new File(_segmentDirectory,
StarTreeV2Constants.EXISTING_STAR_TREE_TEMP_DIR);
+ FileUtils.forceMkdir(_separatorTempDir);
+ FileUtils.copyFileToDirectory(
+ new File(_segmentDirectory, StarTreeV2Constants.INDEX_FILE_NAME),
_separatorTempDir, false);
+ FileUtils.copyFileToDirectory(
+ new File(_segmentDirectory,
StarTreeV2Constants.INDEX_MAP_FILE_NAME), _separatorTempDir, false);
+ return new StarTreeIndexSeparator(new File(_separatorTempDir,
StarTreeV2Constants.INDEX_MAP_FILE_NAME),
+ new File(_separatorTempDir, StarTreeV2Constants.INDEX_FILE_NAME),
_metadataProperties);
+ } catch (Exception e) {
+ try {
+ FileUtils.forceDelete(_separatorTempDir);
+ } catch (Exception e1) {
+ LOGGER.warn(e1.getMessage(), e1);
Review Comment:
(minor) Put a more meaningful message, e.g. `Caught exception when deleting
the separator tmp directory: {}`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java:
##########
@@ -115,13 +126,38 @@ public MultipleTreesBuilder(@Nullable
List<StarTreeIndexConfig> indexConfigs, bo
}
}
+ private StarTreeIndexSeparator getSeparator()
Review Comment:
(minor) Annotate the return as `@Nullable`
```suggestion
@Nullable
private StarTreeIndexSeparator getSeparator()
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java:
##########
@@ -115,13 +126,38 @@ public MultipleTreesBuilder(@Nullable
List<StarTreeIndexConfig> indexConfigs, bo
}
}
+ private StarTreeIndexSeparator getSeparator()
+ throws Exception {
+ if (!_metadataProperties.containsKey(MetadataKey.STAR_TREE_COUNT)) {
+ return null;
+ }
+ try {
+ _separatorTempDir = new File(_segmentDirectory,
StarTreeV2Constants.EXISTING_STAR_TREE_TEMP_DIR);
+ FileUtils.forceMkdir(_separatorTempDir);
+ FileUtils.copyFileToDirectory(
+ new File(_segmentDirectory, StarTreeV2Constants.INDEX_FILE_NAME),
_separatorTempDir, false);
+ FileUtils.copyFileToDirectory(
+ new File(_segmentDirectory,
StarTreeV2Constants.INDEX_MAP_FILE_NAME), _separatorTempDir, false);
Review Comment:
Can we move the file instead of copying? We can move the index files and
only remove the metadata fields
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/MultipleTreesBuilder.java:
##########
@@ -165,6 +231,13 @@ private static SingleTreeBuilder
getSingleTreeBuilder(StarTreeV2BuilderConfig bu
@Override
public void close() {
+ if (_separatorTempDir != null) {
+ try {
+ FileUtils.forceDelete(_separatorTempDir);
+ } catch (Exception e) {
+ LOGGER.warn(e.getMessage(), e);
Review Comment:
(minor) Put a more meaningful message, e.g. `Caught exception when deleting
the separator tmp directory: {}`
--
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
