From 1d807df40160ec8525b5a33847d67a61fef2c54e Mon Sep 17 00:00:00 2001 From: Jialiang Li Date: Tue, 16 Apr 2024 17:03:39 -0700 Subject: [PATCH] Add validation check for forward index disabled if it's a REALTIME table (#12838) --- .../segment/local/utils/TableConfigUtils.java | 20 ++++++++++------ .../local/utils/TableConfigUtilsTest.java | 23 +++++++++++++++++-- 2 files changed, 34 insertions(+), 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 index 14c4040a600..6729f1b027c 100644 --- 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 fieldConfigList, - IndexingConfig indexingConfig, @Nullable Schema schema) { + private static void validateFieldConfigList(TableConfig tableConfig, @Nullable Schema schema) { + List 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 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 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 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/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java index 8f695f2b768..6800f895756 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java +++ b/pinot-segment-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 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 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