Jackie-Jiang commented on code in PR #19349:
URL: https://github.com/apache/pinot/pull/19349#discussion_r3856454594
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -358,19 +361,35 @@ private void removeMultiColumnTextIndex(File indexDir)
private boolean processStarTrees(File indexDir,
@Nullable SegmentOperationsThrottlerSet segmentOperationsThrottlerSet)
throws Exception {
+ SegmentMetadataImpl segmentMetadata =
_segmentDirectory.getSegmentMetadata();
+ String segmentName = segmentMetadata.getName();
+ List<StarTreeV2Metadata> starTreeMetadataList =
segmentMetadata.getStarTreeV2MetadataList();
+
if (!_indexLoadingConfig.isEnableDynamicStarTreeCreation()) {
- return false;
+ // A star-tree whose dimension column is no longer dictionary-encoded
(e.g. because the column was added to
+ // 'noDictionaryColumns' and re-encoded by the forward index handler
above) cannot be read, and fails the whole
+ // segment load. Drop it even here: removing star-trees only deletes
files, so unlike rebuilding them it is
+ // cheap enough to do with dynamic star-tree creation disabled. When it
is enabled the star-trees are rebuilt by
+ // the regular flow below, because their split order no longer matches
the builder configs.
+ Set<String> unloadableDimensions =
+ starTreeMetadataList != null ?
StarTreeBuilderUtils.findUnloadableDimensions(starTreeMetadataList,
+ segmentMetadata) : Set.of();
Review Comment:
(nit, format)
```suggestion
Set<String> unloadableDimensions = starTreeMetadataList != null
?
StarTreeBuilderUtils.findUnloadableDimensions(starTreeMetadataList,
segmentMetadata)
: Set.of();
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/StarTreeBuilderUtils.java:
##########
@@ -283,6 +286,37 @@ public static boolean
shouldModifyExistingStarTrees(List<StarTreeV2BuilderConfig
return false;
}
+ /// Returns the first dimension of the given star-tree that the segment can
no longer back with a dictionary
+ /// encoded forward index, or `null` if the star-tree is loadable.
+ ///
+ /// A star-tree stores its dimension values as dictionary ids in a fixed-bit
forward index whose bit width is read
+ /// from the *main* column metadata at load time. Re-encoding a dimension
column to raw (e.g. after adding it to
+ /// `noDictionaryColumns`) therefore leaves the star-tree unreadable, and
loading the segment fails.
+ @Nullable
+ public static String findUnloadableDimension(StarTreeV2Metadata
starTreeMetadata, SegmentMetadata segmentMetadata) {
+ for (String dimension : starTreeMetadata.getDimensionsSplitOrder()) {
+ ColumnMetadata columnMetadata =
segmentMetadata.getColumnMetadataFor(dimension);
+ if (columnMetadata == null || !columnMetadata.hasDictionary()) {
Review Comment:
@deepthi912 Can you also take a look and see if this works well with the new
added raw dimension support
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java:
##########
@@ -2025,6 +2028,187 @@ public void testStarTreeCreationWithDictionaryChanges()
}
}
+ /// A star-tree dimension column that is moved to 'noDictionaryColumns'
without the star-tree being rebuilt leaves
+ /// the star-tree unreadable: its dimension forward index stores dictionary
ids in a fixed-bit encoding whose width
+ /// is read from the main column metadata, which is now raw. The stale
star-tree must be dropped so the segment
+ /// stays loadable, even when dynamic star-tree creation is disabled.
+ @Test
+ public void testStarTreeDimensionConvertedToNoDictionary()
+ throws Exception {
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build();
+ Schema schema = new
Schema.SchemaBuilder().addSingleValueDimension("stringCol", DataType.STRING)
+ .addMetric("longCol", DataType.LONG)
+ .build();
+ IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
+ indexingConfig.setStarTreeIndexConfigs(
+ List.of(new StarTreeIndexConfig(List.of("stringCol"), null,
List.of("SUM__longCol"), null, 1000)));
+ buildStarTreeTestSegment(tableConfig, schema);
+
+ // Drift the config: the star-tree dimension is moved to
noDictionaryColumns and the star-tree config is dropped,
+ // while dynamic star-tree creation stays disabled.
+ indexingConfig.setNoDictionaryColumns(List.of("stringCol"));
+ indexingConfig.setStarTreeIndexConfigs(null);
+ indexingConfig.setEnableDynamicStarTreeCreation(false);
+ IndexLoadingConfig indexLoadingConfig = new
IndexLoadingConfig(tableConfig, schema);
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap);
+ SegmentPreProcessor processor = new
SegmentPreProcessor(segmentDirectory, indexLoadingConfig)) {
+ assertTrue(processor.needProcess());
+ processor.process(SEGMENT_OPERATIONS_THROTTLER);
+ }
+ assertSegmentLoadsWithoutStarTree(indexLoadingConfig);
+
+ // The stale star-tree is gone, so there is nothing left to process
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap);
+ SegmentPreProcessor processor = new
SegmentPreProcessor(segmentDirectory, indexLoadingConfig)) {
+ assertFalse(processor.needProcess());
+ }
+ }
+
+ /// Same drift as [#testStarTreeDimensionConvertedToNoDictionary()], but the
dict-to-raw conversion has already been
+ /// persisted by an earlier pre-processing round, so the segment on disk is
already inconsistent and nothing else
+ /// needs updating. Pre-processing must still detect and repair it.
+ @Test
+ public void testStarTreeDimensionAlreadyConvertedToNoDictionary()
+ throws Exception {
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build();
+ Schema schema = new
Schema.SchemaBuilder().addSingleValueDimension("stringCol", DataType.STRING)
+ .addMetric("longCol", DataType.LONG)
+ .build();
+ IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
+ indexingConfig.setStarTreeIndexConfigs(
+ List.of(new StarTreeIndexConfig(List.of("stringCol"), null,
List.of("SUM__longCol"), null, 1000)));
+ buildStarTreeTestSegment(tableConfig, schema);
+
+ // Convert the dimension column to raw while leaving the star-tree in
place, reproducing the state an earlier
+ // pre-processing round leaves behind.
+ indexingConfig.setNoDictionaryColumns(List.of("stringCol"));
+ IndexLoadingConfig indexLoadingConfig = new
IndexLoadingConfig(tableConfig, schema);
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap)) {
+ new ForwardIndexHandler(segmentDirectory,
indexLoadingConfig).updateIndices(segmentDirectory.createWriter());
+ }
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap)) {
+
assertFalse(segmentDirectory.getSegmentMetadata().getColumnMetadataFor("stringCol").hasDictionary());
+
assertNotNull(segmentDirectory.getSegmentMetadata().getStarTreeV2MetadataList());
+ }
+
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap);
+ SegmentPreProcessor processor = new
SegmentPreProcessor(segmentDirectory, indexLoadingConfig)) {
+ assertTrue(processor.needProcess());
+ processor.process(SEGMENT_OPERATIONS_THROTTLER);
+ }
+ assertSegmentLoadsWithoutStarTree(indexLoadingConfig);
+ }
+
+ /// The loader must not fail the whole segment over a stale star-tree even
when pre-processing never gets a chance to
+ /// repair it, e.g. because it is skipped for the table.
+ @Test
+ public void testStarTreeDimensionConvertedToNoDictionaryWithoutPreprocess()
+ throws Exception {
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build();
+ Schema schema = new
Schema.SchemaBuilder().addSingleValueDimension("stringCol", DataType.STRING)
+ .addMetric("longCol", DataType.LONG)
+ .build();
+ IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
+ indexingConfig.setStarTreeIndexConfigs(
+ List.of(new StarTreeIndexConfig(List.of("stringCol"), null,
List.of("SUM__longCol"), null, 1000)));
+ buildStarTreeTestSegment(tableConfig, schema);
+
+ indexingConfig.setNoDictionaryColumns(List.of("stringCol"));
+ IndexLoadingConfig indexLoadingConfig = new
IndexLoadingConfig(tableConfig, schema);
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap)) {
+ new ForwardIndexHandler(segmentDirectory,
indexLoadingConfig).updateIndices(segmentDirectory.createWriter());
+ }
+
+ // The stale star-tree is still in the segment, but it must be skipped
rather than fail the load
+ ImmutableSegment segment = ImmutableSegmentLoader.load(INDEX_DIR,
indexLoadingConfig, false);
+ try {
+ assertEquals(segment.getSegmentMetadata().getTotalDocs(), 5);
+ assertTrue(segment.getStarTrees() == null ||
segment.getStarTrees().isEmpty());
+ } finally {
+ segment.destroy();
+ }
+ }
+
+ /// With dynamic star-tree creation enabled, the stale star-tree is not just
dropped but rebuilt from the current
+ /// config, which no longer splits on the re-encoded column.
+ @Test
+ public void testStarTreeDimensionConvertedToNoDictionaryWithDynamicCreation()
+ throws Exception {
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build();
+ Schema schema = new
Schema.SchemaBuilder().addSingleValueDimension("stringCol", DataType.STRING)
+ .addSingleValueDimension("intCol", DataType.INT)
+ .addMetric("longCol", DataType.LONG)
+ .build();
+ IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
+ indexingConfig.setStarTreeIndexConfigs(
+ List.of(new StarTreeIndexConfig(List.of("stringCol", "intCol"), null,
List.of("SUM__longCol"), null, 1000)));
+ buildStarTreeTestSegment(tableConfig, schema);
+
+ // 'stringCol' becomes raw and drops out of the split order, and the
star-tree is rebuilt on 'intCol' alone
+ indexingConfig.setNoDictionaryColumns(List.of("stringCol"));
+ indexingConfig.setStarTreeIndexConfigs(
+ List.of(new StarTreeIndexConfig(List.of("intCol"), null,
List.of("SUM__longCol"), null, 1000)));
+ indexingConfig.setEnableDynamicStarTreeCreation(true);
+ IndexLoadingConfig indexLoadingConfig = new
IndexLoadingConfig(tableConfig, schema);
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap);
+ SegmentPreProcessor processor = new
SegmentPreProcessor(segmentDirectory, indexLoadingConfig)) {
+ assertTrue(processor.needProcess());
+ processor.process(SEGMENT_OPERATIONS_THROTTLER);
+ }
+
+ ImmutableSegment segment = ImmutableSegmentLoader.load(INDEX_DIR,
indexLoadingConfig, false);
+ try {
+ List<StarTreeV2> starTrees = segment.getStarTrees();
+ assertNotNull(starTrees);
+ assertEquals(starTrees.size(), 1);
+ assertEquals(starTrees.get(0).getMetadata().getDimensionsSplitOrder(),
List.of("intCol"));
Review Comment:
#19153 added support for star-tree on raw encoded column with separate
dictionary. We should verify if the preprocess can rebuild the star-tree to
generate dictionary.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
