Fix regression in ForwardIndexType detected in apache#11745 (apache#1…

…1784)
walterddr · Oct 14, 2023 · d7d504b · d7d504b
1 parent 1e04c8a
commit d7d504b
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 13 deletions.
diff --git a/.../src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java b/.../src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java
@@ -20,8 +20,10 @@
 package org.apache.pinot.segment.local.segment.index.forward;
 
 import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import java.io.IOException;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -41,6 +43,7 @@
 import org.apache.pinot.segment.spi.creator.IndexCreationContext;
 import org.apache.pinot.segment.spi.index.AbstractIndexType;
 import org.apache.pinot.segment.spi.index.ColumnConfigDeserializer;
+import org.apache.pinot.segment.spi.index.DictionaryIndexConfig;
 import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
 import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
 import org.apache.pinot.segment.spi.index.IndexConfigDeserializer;
@@ -153,24 +156,35 @@ public String getPrettyName() {
   @Override
   public ColumnConfigDeserializer<ForwardIndexConfig> createDeserializer() {
     // reads tableConfig.fieldConfigList and decides what to create using the FieldConfig properties and encoding
-    ColumnConfigDeserializer<ForwardIndexConfig> fromFieldConfig = IndexConfigDeserializer.fromCollection(
-        TableConfig::getFieldConfigList,
-        (accum, fieldConfig) -> {
+    ColumnConfigDeserializer<ForwardIndexConfig> fromOld = (tableConfig, schema) -> {
+      Map<String, DictionaryIndexConfig> dictConfigs = StandardIndexes.dictionary().getConfig(tableConfig, schema);
+
+      Map<String, ForwardIndexConfig> fwdConfig = Maps.newHashMapWithExpectedSize(
+          Math.max(dictConfigs.size(), schema.size()));
+
+      Collection<FieldConfig> fieldConfigs = tableConfig.getFieldConfigList();
+      if (fieldConfigs != null) {
+        for (FieldConfig fieldConfig : fieldConfigs) {
           Map<String, String> properties = fieldConfig.getProperties();
           if (properties != null && isDisabled(properties)) {
-            accum.put(fieldConfig.getName(), ForwardIndexConfig.DISABLED);
-          } else if (fieldConfig.getEncodingType() == FieldConfig.EncodingType.RAW) {
-            accum.put(fieldConfig.getName(), createConfigFromFieldConfig(fieldConfig));
+            fwdConfig.put(fieldConfig.getName(), ForwardIndexConfig.DISABLED);
+          } else {
+            DictionaryIndexConfig dictConfig = dictConfigs.get(fieldConfig.getName());
+            if (dictConfig != null && dictConfig.isDisabled()) {
+              fwdConfig.put(fieldConfig.getName(), createConfigFromFieldConfig(fieldConfig));
+            }
+            // On other case encoding is DICTIONARY. We create the default forward index by default. That means that if
+            // field config indicates for example a compression while using encoding dictionary, the compression is
+            // ignored.
+            // It is important to do not explicitly add the default value here in order to avoid exclusive problems with
+            // the default `fromIndexes` deserializer.
           }
-          // On other case encoding is DICTIONARY. We create the default forward index by default. That means that if
-          // field config indicates for example a compression while using encoding dictionary, the compression is
-          // ignored.
-          // It is important to do not explicitly add the default value here in order to avoid exclusive problems with
-          // the default `fromIndexes` deserializer.
         }
-    );
+      }
+      return fwdConfig;
+    };
     return IndexConfigDeserializer.fromIndexes(getPrettyName(), getIndexConfigClass())
-        .withExclusiveAlternative(fromFieldConfig);
+        .withExclusiveAlternative(fromOld);
   }
 
   private boolean isDisabled(Map<String, String> props) {

diff --git a/.../test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexTypeTest.java b/.../test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexTypeTest.java
@@ -20,7 +20,9 @@
 package org.apache.pinot.segment.local.segment.index.forward;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.core.type.TypeReference;
 import java.io.IOException;
+import java.util.Map;
 import java.util.stream.Collectors;
 import org.apache.pinot.segment.local.segment.index.AbstractSerdeIndexContract;
 import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
@@ -102,6 +104,51 @@ public void oldConfEnableDefault()
       assertEquals(ForwardIndexConfig.DEFAULT);
     }
 
+    @Test
+    public void oldConfNoDictionaryConfig()
+        throws IOException {
+      _tableConfig.getIndexingConfig().setNoDictionaryConfig(
+          JsonUtils.stringToObject("{\"dimInt\": \"RAW\"}",
+              new TypeReference<Map<String, String>>() {
+              })
+      );
+      addFieldIndexConfig(""
+          + " {\n"
+          + "    \"name\": \"dimInt\","
+          + "    \"compressionCodec\": \"SNAPPY\"\n"
+          + " }"
+      );
+
+      assertEquals(
+          new ForwardIndexConfig.Builder()
+              .withCompressionType(ChunkCompressionType.SNAPPY)
+              .withDeriveNumDocsPerChunk(false)
+              .withRawIndexWriterVersion(2)
+              .build()
+      );
+    }
+
+    @Test
+    public void oldConfNoDictionaryColumns()
+        throws IOException {
+      _tableConfig.getIndexingConfig().setNoDictionaryColumns(
+          JsonUtils.stringToObject("[\"dimInt\"]", _stringListTypeRef));
+      addFieldIndexConfig(""
+          + " {\n"
+          + "    \"name\": \"dimInt\","
+          + "    \"compressionCodec\": \"SNAPPY\"\n"
+          + " }"
+      );
+
+      assertEquals(
+          new ForwardIndexConfig.Builder()
+              .withCompressionType(ChunkCompressionType.SNAPPY)
+              .withDeriveNumDocsPerChunk(false)
+              .withRawIndexWriterVersion(2)
+              .build()
+      );
+    }
+
     @Test
     public void oldConfEnableDict()
         throws IOException {