Add validation check for forward index disabled if it's a REALTIME ta…

…ble (apache#12838)
soumitra-st · Apr 17, 2024 · 1d807df · 1d807df
1 parent 67cb52c
commit 1d807df
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 9 deletions.
diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
@@ -173,7 +173,7 @@ public static void validate(TableConfig tableConfig, @Nullable Schema schema, @N
       }
       validateTierConfigList(tableConfig.getTierConfigsList());
       validateIndexingConfig(tableConfig.getIndexingConfig(), schema);
-      validateFieldConfigList(tableConfig.getFieldConfigList(), tableConfig.getIndexingConfig(), schema);
+      validateFieldConfigList(tableConfig, schema);
       validateInstancePartitionsTypeMapConfig(tableConfig);
       validatePartitionedReplicaGroupInstance(tableConfig);
       if (!skipTypes.contains(ValidationType.UPSERT)) {
@@ -1209,8 +1209,10 @@ private static void validateIndexingConfig(IndexingConfig indexingConfig, @Nulla
    * Additional checks for TEXT and FST index types
    * Validates index compatibility for forward index disabled columns
    */
-  private static void validateFieldConfigList(@Nullable List<FieldConfig> fieldConfigList,
-      IndexingConfig indexingConfig, @Nullable Schema schema) {
+  private static void validateFieldConfigList(TableConfig tableConfig, @Nullable Schema schema) {
+    List<FieldConfig> fieldConfigList = tableConfig.getFieldConfigList();
+    IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
+    TableType tableType = tableConfig.getTableType();
     if (fieldConfigList == null) {
       return;
     }
@@ -1254,7 +1256,7 @@ private static void validateFieldConfigList(@Nullable List<FieldConfig> fieldCon
           "Column: %s defined in field config list must be a valid column defined in the schema", columnName);
 
       // Validate the forward index disabled compatibility with other indexes if enabled for this column
-      validateForwardIndexDisabledIndexCompatibility(columnName, fieldConfig, indexingConfig, schema);
+      validateForwardIndexDisabledIndexCompatibility(columnName, fieldConfig, indexingConfig, schema, tableType);
 
       if (CollectionUtils.isNotEmpty(fieldConfig.getIndexTypes())) {
         for (FieldConfig.IndexType indexType : fieldConfig.getIndexTypes()) {
@@ -1300,7 +1302,7 @@ private static void validateFieldConfigList(@Nullable List<FieldConfig> fieldCon
    * back or generate a new index for existing segments is to either refresh or back-fill the segments.
    */
   private static void validateForwardIndexDisabledIndexCompatibility(String columnName, FieldConfig fieldConfig,
-      IndexingConfig indexingConfig, Schema schema) {
+      IndexingConfig indexingConfig, Schema schema, TableType tableType) {
     Map<String, String> fieldConfigProperties = fieldConfig.getProperties();
     if (fieldConfigProperties == null) {
       return;
@@ -1313,16 +1315,20 @@ private static void validateForwardIndexDisabledIndexCompatibility(String column
       return;
     }
 
+    // For tables with columnMajorSegmentBuilderEnabled being true, the forward index should not be disabled.
+    Preconditions.checkState(tableType != TableType.REALTIME,
+        String.format("Cannot disable forward index for column %s, as the table type is REALTIME.", columnName));
+
     FieldSpec fieldSpec = schema.getFieldSpecFor(columnName);
     // Check for the range index since the index itself relies on the existence of the forward index to work.
     if (indexingConfig.getRangeIndexColumns() != null && indexingConfig.getRangeIndexColumns().contains(columnName)) {
       Preconditions.checkState(fieldSpec.isSingleValueField(), String.format("Feature not supported for multi-value "
           + "columns with range index. Cannot disable forward index for column %s. Disable range index on this "
-          + "column to use this feature", columnName));
+          + "column to use this feature.", columnName));
       Preconditions.checkState(indexingConfig.getRangeIndexVersion() == BitSlicedRangeIndexCreator.VERSION,
           String.format("Feature not supported for single-value columns with range index version < 2. Cannot disable "
               + "forward index for column %s. Either disable range index or create range index with"
-              + " version >= 2 to use this feature", columnName));
+              + " version >= 2 to use this feature.", columnName));
     }
 
     Preconditions.checkState(!indexingConfig.isOptimizeDictionaryForMetrics() && !indexingConfig.isOptimizeDictionary(),

diff --git a/...egment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java b/...egment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java
@@ -989,6 +989,7 @@ public void testTableName() {
   @Test
   public void testValidateFieldConfig() {
     Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME)
+        .addDateTime(TIME_COLUMN, FieldSpec.DataType.LONG, "1:HOURS:EPOCH", "1:HOURS")
         .addSingleValueDimension("myCol1", FieldSpec.DataType.STRING)
         .addMultiValueDimension("myCol2", FieldSpec.DataType.INT)
         .addSingleValueDimension("intCol", FieldSpec.DataType.INT).build();
@@ -1190,7 +1191,7 @@ public void testValidateFieldConfig() {
       Assert.fail("Should fail for MV myCol2 with forward index disabled but has range and inverted index");
     } catch (Exception e) {
       Assert.assertEquals(e.getMessage(), "Feature not supported for multi-value columns with range index. "
-          + "Cannot disable forward index for column myCol2. Disable range index on this column to use this feature");
+          + "Cannot disable forward index for column myCol2. Disable range index on this column to use this feature.");
     }
 
     tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
@@ -1210,7 +1211,7 @@ public void testValidateFieldConfig() {
     } catch (Exception e) {
       Assert.assertEquals(e.getMessage(), "Feature not supported for single-value columns with range index version "
           + "< 2. Cannot disable forward index for column myCol1. Either disable range index or create range index "
-          + "with version >= 2 to use this feature");
+          + "with version >= 2 to use this feature.");
     }
 
     tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
@@ -1277,6 +1278,24 @@ public void testValidateFieldConfig() {
     } catch (Exception e) {
       Assert.fail("Range index with forward index disabled no dictionary column is allowed");
     }
+
+    // Disabling forward index for realtime table will make the validation failed.
+    Map<String, String> streamConfigs = getStreamConfigs();
+    tableConfig = new TableConfigBuilder(TableType.REALTIME).setTableName(TABLE_NAME).setTimeColumnName(TIME_COLUMN)
+        .setNoDictionaryColumns(Arrays.asList("intCol")).setStreamConfigs(streamConfigs).build();
+    try {
+      // Enable forward index disabled flag for a column with inverted index index and disable dictionary
+      Map<String, String> fieldConfigProperties = new HashMap<>();
+      fieldConfigProperties.put(FieldConfig.FORWARD_INDEX_DISABLED, Boolean.TRUE.toString());
+      FieldConfig fieldConfig =
+          new FieldConfig("intCol", FieldConfig.EncodingType.RAW, FieldConfig.IndexType.INVERTED, null, null, null,
+              fieldConfigProperties);
+      tableConfig.setFieldConfigList(Arrays.asList(fieldConfig));
+      TableConfigUtils.validate(tableConfig, schema);
+    } catch (Exception e) {
+      Assert.assertEquals(e.getMessage(),
+          "Cannot disable forward index for column intCol, as the table type is REALTIME.");
+    }
   }
 
   @Test