Drop star-tree indexes left unreadable by a dimension encoding change

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/reader/impl/FixedBitIntReader.java

@@ -106,7 +106,8 @@ public static FixedBitIntReader getReader(PinotDataBuffer dataBuffer, int numBit
       case 31:
         return new Bit31Reader(dataBuffer);
       default:
-        throw new IllegalStateException();
+        throw new IllegalStateException("Illegal number of bits per value: " + numBitsPerValue + ", must be within 1 "
+            + "and 31");
     }
   }
 

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java

@@ -23,6 +23,7 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 import javax.annotation.Nullable;
 import org.apache.commons.configuration2.PropertiesConfiguration;
 import org.apache.commons.configuration2.ex.ConfigurationException;
@@ -256,16 +257,18 @@ private List<String> columnMinMaxValueUpdates() {
   }
 
   private boolean needProcessStarTrees() {
+    SegmentMetadataImpl segmentMetadata = _segmentDirectory.getSegmentMetadata();
+    List<StarTreeV2Metadata> starTreeMetadataList = segmentMetadata.getStarTreeV2MetadataList();
     // Check if there is need to create/modify/remove star-trees.
     if (!_indexLoadingConfig.isEnableDynamicStarTreeCreation()) {
-      return false;
+      // Star-trees left unreadable by a column encoding change are still removed, see processStarTrees().
+      return starTreeMetadataList != null && !StarTreeBuilderUtils.findUnloadableDimensions(starTreeMetadataList,
+          segmentMetadata).isEmpty();
     }
 
-    SegmentMetadataImpl segmentMetadata = _segmentDirectory.getSegmentMetadata();
     List<StarTreeV2BuilderConfig> starTreeBuilderConfigs =
         StarTreeBuilderUtils.generateBuilderConfigs(_indexLoadingConfig.getStarTreeIndexConfigs(),
             _indexLoadingConfig.isEnableDefaultStarTree(), segmentMetadata);
-    List<StarTreeV2Metadata> starTreeMetadataList = segmentMetadata.getStarTreeV2MetadataList();
     // There are existing star-trees, but if they match the builder configs exactly,
     // then there is no need to generate the star-trees
 
@@ -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();
+      if (unloadableDimensions.isEmpty()) {
+        return false;
+      }
+      LOGGER.warn("Removing star-trees from segment: {} because dimension columns: {} are no longer "
+              + "dictionary-encoded. Enable dynamic star-tree creation to have them rebuilt", segmentName,
+          unloadableDimensions);
+      StarTreeBuilderUtils.removeStarTrees(indexDir);
+      return true;
     }
 
-    SegmentMetadataImpl segmentMetadata = _segmentDirectory.getSegmentMetadata();
-    String segmentName = segmentMetadata.getName();
     List<StarTreeV2BuilderConfig> starTreeBuilderConfigs =
         StarTreeBuilderUtils.generateBuilderConfigs(_indexLoadingConfig.getStarTreeIndexConfigs(),
             _indexLoadingConfig.isEnableDefaultStarTree(), segmentMetadata);
 
     boolean shouldGenerateStarTree = !starTreeBuilderConfigs.isEmpty();
     boolean shouldRemoveStarTree = false;
-    List<StarTreeV2Metadata> starTreeMetadataList = segmentMetadata.getStarTreeV2MetadataList();
     if (starTreeMetadataList != null) {
       // There are existing star-trees
       if (!shouldGenerateStarTree) {

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/StarTreeBuilderUtils.java

@@ -28,14 +28,17 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Queue;
+import java.util.Set;
 import java.util.TreeMap;
+import java.util.TreeSet;
 import javax.annotation.Nullable;
 import org.apache.commons.configuration2.PropertiesConfiguration;
 import org.apache.commons.io.FileUtils;
 import org.apache.pinot.common.request.Literal;
 import org.apache.pinot.common.request.context.ExpressionContext;
 import org.apache.pinot.segment.local.startree.v2.builder.StarTreeV2BuilderConfig;
 import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.apache.pinot.segment.spi.ColumnMetadata;
 import org.apache.pinot.segment.spi.Constants;
 import org.apache.pinot.segment.spi.SegmentMetadata;
 import org.apache.pinot.segment.spi.index.startree.AggregationFunctionColumnPair;
@@ -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()) {
+        return dimension;
+      }
+    }
+    return null;
+  }
+
+  /// Returns the dimensions that make the given star-trees unloadable, or an empty set if they are all loadable.
+  /// See [#findUnloadableDimension(StarTreeV2Metadata, SegmentMetadata)].
+  public static Set<String> findUnloadableDimensions(List<StarTreeV2Metadata> metadataList,
+      SegmentMetadata segmentMetadata) {
+    Set<String> dimensions = new TreeSet<>();
+    for (StarTreeV2Metadata starTreeMetadata : metadataList) {
+      String dimension = findUnloadableDimension(starTreeMetadata, segmentMetadata);
+      if (dimension != null) {
+        dimensions.add(dimension);
+      }
+    }
+    return dimensions;
+  }
+
   /// Returns `true` if the given star-tree builder configs are equal, `false` otherwise.
   public static boolean areStarTreeBuilderConfigListsEqual(List<StarTreeV2BuilderConfig> builderConfig1,
       List<StarTreeV2BuilderConfig> builderConfig2) {

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/store/StarTreeLoaderUtils.java

@@ -27,6 +27,7 @@
 import org.apache.pinot.segment.local.segment.index.forward.ForwardIndexReaderFactory;
 import org.apache.pinot.segment.local.segment.index.readers.forward.FixedBitSVForwardIndexReaderV2;
 import org.apache.pinot.segment.local.startree.OffHeapStarTree;
+import org.apache.pinot.segment.local.startree.StarTreeBuilderUtils;
 import org.apache.pinot.segment.spi.ColumnMetadata;
 import org.apache.pinot.segment.spi.datasource.DataSource;
 import org.apache.pinot.segment.spi.index.StandardIndexes;
@@ -42,10 +43,14 @@
 import org.apache.pinot.spi.data.FieldSpec;
 import org.apache.pinot.spi.data.FieldSpec.DataType;
 import org.apache.pinot.spi.data.MetricFieldSpec;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 
 /// The `StarTreeLoaderUtils` class provides utility methods to load star-tree indexes.
 public class StarTreeLoaderUtils {
+  private static final Logger LOGGER = LoggerFactory.getLogger(StarTreeLoaderUtils.class);
+
   private StarTreeLoaderUtils() {
   }
 
@@ -57,11 +62,23 @@ public static List<StarTreeV2> loadStarTreeV2(SegmentDirectory.Reader segmentRea
     int numStarTrees = starTreeMetadataList.size();
     List<StarTreeV2> starTrees = new ArrayList<>(numStarTrees);
     for (int i = 0; i < numStarTrees; i++) {
+      StarTreeV2Metadata starTreeMetadata = starTreeMetadataList.get(i);
+
+      // A star-tree is unreadable once one of its dimension columns loses its dictionary, which happens when the
+      // column is moved to 'noDictionaryColumns' without the star-tree being rebuilt. Skip it instead of failing the
+      // whole segment load. SegmentPreProcessor normally removes such star-trees, so reaching this point means the
+      // pre-processing was skipped for this segment.
+      String unloadableDimension = StarTreeBuilderUtils.findUnloadableDimension(starTreeMetadata, segmentMetadata);
+      if (unloadableDimension != null) {
+        LOGGER.warn("Skipping star-tree: {} in segment: {} because dimension column: {} is no longer "
+            + "dictionary-encoded", i, segmentMetadata.getName(), unloadableDimension);
+        continue;
+      }
+
       SegmentDirectory.Reader indexReader = segmentReader.getStarTreeIndexReader(i);
       // Load star-tree index
       StarTree starTree = new OffHeapStarTree(indexReader.getIndexFor(String.valueOf(i), StandardIndexes.inverted()));
 
-      StarTreeV2Metadata starTreeMetadata = starTreeMetadataList.get(i);
       int numDocs = starTreeMetadata.getNumDocs();
       Map<String, DataSource> dataSourceMap = new HashMap<>();