addressing review comments

Signed-off-by: Bharathwaj G <[email protected]>

server/src/main/java/org/opensearch/index/IndexModule.java

@@ -312,6 +312,7 @@ public Iterator<Setting<?>> settings() {
     private final BooleanSupplier allowExpensiveQueries;
     private final Map<String, IndexStorePlugin.RecoveryStateFactory> recoveryStateFactories;
     private final FileCache fileCache;
+    private final CompositeIndexSettings compositeIndexSettings;
 
     /**
      * Construct the index module for the index with the specified index settings. The index module contains extension points for plugins
@@ -331,7 +332,8 @@ public IndexModule(
         final BooleanSupplier allowExpensiveQueries,
         final IndexNameExpressionResolver expressionResolver,
         final Map<String, IndexStorePlugin.RecoveryStateFactory> recoveryStateFactories,
-        final FileCache fileCache
+        final FileCache fileCache,
+        final CompositeIndexSettings compositeIndexSettings
     ) {
         this.indexSettings = indexSettings;
         this.analysisRegistry = analysisRegistry;
@@ -344,6 +346,7 @@ public IndexModule(
         this.expressionResolver = expressionResolver;
         this.recoveryStateFactories = recoveryStateFactories;
         this.fileCache = fileCache;
+        this.compositeIndexSettings = compositeIndexSettings;
     }
 
     public IndexModule(
@@ -365,6 +368,7 @@ public IndexModule(
             allowExpensiveQueries,
             expressionResolver,
             recoveryStateFactories,
+            null,
             null
         );
     }
@@ -680,8 +684,7 @@ public IndexService newIndexService(
         BiFunction<IndexSettings, ShardRouting, TranslogFactory> translogFactorySupplier,
         Supplier<TimeValue> clusterDefaultRefreshIntervalSupplier,
         RecoverySettings recoverySettings,
-        RemoteStoreSettings remoteStoreSettings,
-        CompositeIndexSettings compositeIndexSettings
+        RemoteStoreSettings remoteStoreSettings
     ) throws IOException {
         final IndexEventListener eventListener = freeze();
         Function<IndexService, CheckedFunction<DirectoryReader, DirectoryReader, IOException>> readerWrapperFactory = indexReaderWrapper

server/src/main/java/org/opensearch/index/compositeindex/CompositeIndexSettings.java

@@ -52,16 +52,4 @@ private void starTreeIndexCreationEnabled(boolean value) {
     public boolean isStarTreeIndexCreationEnabled() {
         return starTreeIndexCreationEnabled;
     }
-
-    /**
-     * Utility to provide a {@link CompositeIndexSettings} instance containing all defaults
-     */
-    public static class DefaultCompositeIndexSettings {
-        private DefaultCompositeIndexSettings() {}
-
-        public static final CompositeIndexSettings INSTANCE = new CompositeIndexSettings(
-            Settings.EMPTY,
-            new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)
-        );
-    }
 }

server/src/main/java/org/opensearch/index/compositeindex/CompositeIndexValidator.java

@@ -13,7 +13,6 @@
 import org.opensearch.index.compositeindex.datacube.Dimension;
 import org.opensearch.index.compositeindex.datacube.Metric;
 import org.opensearch.index.compositeindex.datacube.startree.StarTreeIndexSettings;
-import org.opensearch.index.mapper.CompositeDataCubeFieldType;
 import org.opensearch.index.mapper.CompositeMappedFieldType;
 import org.opensearch.index.mapper.MappedFieldType;
 import org.opensearch.index.mapper.MapperService;
@@ -57,9 +56,7 @@ private static void validateStarTreeMappings(
         IndexSettings indexSettings
     ) {
         Set<CompositeMappedFieldType> compositeFieldTypes = mapperService.getCompositeFieldTypes();
-        if (mapperService.getCompositeFieldTypes().size() > StarTreeIndexSettings.STAR_TREE_MAX_FIELDS_SETTING.get(
-            indexSettings.getSettings()
-        )) {
+        if (compositeFieldTypes.size() > StarTreeIndexSettings.STAR_TREE_MAX_FIELDS_SETTING.get(indexSettings.getSettings())) {
             throw new IllegalArgumentException(
                 String.format(
                     Locale.ROOT,
@@ -70,7 +67,7 @@ private static void validateStarTreeMappings(
         }
         for (CompositeMappedFieldType compositeFieldType : compositeFieldTypes) {
             if (!(compositeFieldType instanceof StarTreeMapper.StarTreeFieldType)) {
-                return;
+                continue;
             }
             if (!compositeIndexSettings.isStarTreeIndexCreationEnabled()) {
                 throw new IllegalArgumentException(
@@ -81,15 +78,15 @@ private static void validateStarTreeMappings(
                     )
                 );
             }
-            CompositeDataCubeFieldType dataCubeFieldType = (CompositeDataCubeFieldType) compositeFieldType;
+            StarTreeMapper.StarTreeFieldType dataCubeFieldType = (StarTreeMapper.StarTreeFieldType) compositeFieldType;
             for (Dimension dim : dataCubeFieldType.getDimensions()) {
                 MappedFieldType ft = mapperService.fieldType(dim.getField());
                 if (ft == null) {
                     throw new IllegalArgumentException(
                         String.format(Locale.ROOT, "unknown dimension field [%s] as part of star tree field", dim.getField())
                     );
                 }
-                if (!ft.isAggregatable()) {
+                if (ft.isAggregatable() == false) {
                     throw new IllegalArgumentException(
                         String.format(
                             Locale.ROOT,
@@ -107,7 +104,7 @@ private static void validateStarTreeMappings(
                         String.format(Locale.ROOT, "unknown metric field [%s] as part of star tree field", metric.getField())
                     );
                 }
-                if (!ft.isAggregatable()) {
+                if (ft.isAggregatable() == false) {
                     throw new IllegalArgumentException(
                         String.format(
                             Locale.ROOT,

server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/StarTreeFieldConfiguration.java

@@ -13,9 +13,9 @@
 import org.opensearch.core.xcontent.XContentBuilder;
 
 import java.io.IOException;
-import java.util.List;
 import java.util.Locale;
 import java.util.Objects;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 
 /**
@@ -27,10 +27,10 @@
 public class StarTreeFieldConfiguration implements ToXContent {
 
     private final AtomicInteger maxLeafDocs = new AtomicInteger();
-    private final List<String> skipStarNodeCreationInDims;
+    private final Set<String> skipStarNodeCreationInDims;
     private final StarTreeBuildMode buildMode;
 
-    public StarTreeFieldConfiguration(int maxLeafDocs, List<String> skipStarNodeCreationInDims, StarTreeBuildMode buildMode) {
+    public StarTreeFieldConfiguration(int maxLeafDocs, Set<String> skipStarNodeCreationInDims, StarTreeBuildMode buildMode) {
         this.maxLeafDocs.set(maxLeafDocs);
         this.skipStarNodeCreationInDims = skipStarNodeCreationInDims;
         this.buildMode = buildMode;
@@ -87,7 +87,7 @@ public StarTreeBuildMode getBuildMode() {
         return buildMode;
     }
 
-    public List<String> getSkipStarNodeCreationInDims() {
+    public Set<String> getSkipStarNodeCreationInDims() {
         return skipStarNodeCreationInDims;
     }
 

server/src/main/java/org/opensearch/index/mapper/StarTreeMapper.java

@@ -22,7 +22,6 @@
 import org.opensearch.search.lookup.SearchLookup;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
@@ -83,8 +82,8 @@ public static class Builder extends ParametrizedFieldMapper.Builder {
                     );
                 }
                 paramMap.remove(MAX_LEAF_DOCS);
-                List<String> skipStarInDims = Arrays.asList(
-                    XContentMapValues.nodeStringArrayValue(paramMap.getOrDefault(SKIP_STAR_NODE_IN_DIMS, new ArrayList<String>()))
+                Set<String> skipStarInDims = new LinkedHashSet<>(
+                    List.of(XContentMapValues.nodeStringArrayValue(paramMap.getOrDefault(SKIP_STAR_NODE_IN_DIMS, new ArrayList<String>())))
                 );
                 paramMap.remove(SKIP_STAR_NODE_IN_DIMS);
                 StarTreeFieldConfiguration.StarTreeBuildMode buildMode = StarTreeFieldConfiguration.StarTreeBuildMode.fromTypeName(

server/src/main/java/org/opensearch/indices/IndicesService.java

@@ -944,7 +944,8 @@ private synchronized IndexService createIndexService(
             () -> allowExpensiveQueries,
             indexNameExpressionResolver,
             recoveryStateFactories,
-            fileCache
+            fileCache,
+            compositeIndexSettings
         );
         for (IndexingOperationListener operationListener : indexingOperationListeners) {
             indexModule.addIndexOperationListener(operationListener);
@@ -974,8 +975,7 @@ private synchronized IndexService createIndexService(
             translogFactorySupplier,
             this::getClusterDefaultRefreshInterval,
             this.recoverySettings,
-            this.remoteStoreSettings,
-            this.compositeIndexSettings
+            this.remoteStoreSettings
         );
     }
 
@@ -1036,7 +1036,8 @@ public synchronized MapperService createIndexMapperService(IndexMetadata indexMe
             () -> allowExpensiveQueries,
             indexNameExpressionResolver,
             recoveryStateFactories,
-            fileCache
+            fileCache,
+            compositeIndexSettings
         );
         pluginsService.onIndexModule(indexModule);
         return indexModule.newIndexMapperService(xContentRegistry, mapperRegistry, scriptService);

server/src/test/java/org/opensearch/index/IndexModuleTests.java

@@ -80,7 +80,6 @@
 import org.opensearch.index.cache.query.DisabledQueryCache;
 import org.opensearch.index.cache.query.IndexQueryCache;
 import org.opensearch.index.cache.query.QueryCache;
-import org.opensearch.index.compositeindex.CompositeIndexSettings;
 import org.opensearch.index.engine.Engine;
 import org.opensearch.index.engine.EngineConfigFactory;
 import org.opensearch.index.engine.InternalEngineFactory;
@@ -265,8 +264,7 @@ private IndexService newIndexService(IndexModule module) throws IOException {
             translogFactorySupplier,
             () -> IndexSettings.DEFAULT_REFRESH_INTERVAL,
             DefaultRecoverySettings.INSTANCE,
-            DefaultRemoteStoreSettings.INSTANCE,
-            CompositeIndexSettings.DefaultCompositeIndexSettings.INSTANCE
+            DefaultRemoteStoreSettings.INSTANCE
         );
     }
 

server/src/test/java/org/opensearch/index/compositeindex/datacube/startree/StarTreeMappingIntegTests.java

@@ -227,7 +227,7 @@ public void testValidCompositeIndex() {
                         StarTreeFieldConfiguration.StarTreeBuildMode.OFF_HEAP,
                         starTreeFieldType.getStarTreeConfig().getBuildMode()
                     );
-                    assertEquals(Collections.emptyList(), starTreeFieldType.getStarTreeConfig().getSkipStarNodeCreationInDims());
+                    assertEquals(Collections.emptySet(), starTreeFieldType.getStarTreeConfig().getSkipStarNodeCreationInDims());
                 }
             }
         }
@@ -311,7 +311,7 @@ public void testUpdateIndexWhenMappingIsSame() {
                         StarTreeFieldConfiguration.StarTreeBuildMode.OFF_HEAP,
                         starTreeFieldType.getStarTreeConfig().getBuildMode()
                     );
-                    assertEquals(Collections.emptyList(), starTreeFieldType.getStarTreeConfig().getSkipStarNodeCreationInDims());
+                    assertEquals(Collections.emptySet(), starTreeFieldType.getStarTreeConfig().getSkipStarNodeCreationInDims());
                 }
             }
         }

server/src/test/java/org/opensearch/index/mapper/StarTreeMapperTests.java

@@ -22,6 +22,7 @@
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
@@ -61,7 +62,10 @@ public void testValidStarTree() throws IOException {
             assertEquals(expectedMetrics, starTreeFieldType.getMetrics().get(0).getMetrics());
             assertEquals(100, starTreeFieldType.getStarTreeConfig().maxLeafDocs());
             assertEquals(StarTreeFieldConfiguration.StarTreeBuildMode.ON_HEAP, starTreeFieldType.getStarTreeConfig().getBuildMode());
-            assertEquals(Arrays.asList("@timestamp", "status"), starTreeFieldType.getStarTreeConfig().getSkipStarNodeCreationInDims());
+            assertEquals(
+                new HashSet<>(Arrays.asList("@timestamp", "status")),
+                starTreeFieldType.getStarTreeConfig().getSkipStarNodeCreationInDims()
+            );
         }
     }
 
@@ -90,7 +94,7 @@ public void testValidStarTreeDefaults() throws IOException {
             assertEquals(expectedMetrics, starTreeFieldType.getMetrics().get(0).getMetrics());
             assertEquals(10000, starTreeFieldType.getStarTreeConfig().maxLeafDocs());
             assertEquals(StarTreeFieldConfiguration.StarTreeBuildMode.OFF_HEAP, starTreeFieldType.getStarTreeConfig().getBuildMode());
-            assertEquals(Collections.emptyList(), starTreeFieldType.getStarTreeConfig().getSkipStarNodeCreationInDims());
+            assertEquals(Collections.emptySet(), starTreeFieldType.getStarTreeConfig().getSkipStarNodeCreationInDims());
         }
     }
 

server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java

@@ -185,7 +185,6 @@
 import org.opensearch.index.SegmentReplicationPressureService;
 import org.opensearch.index.SegmentReplicationStatsTracker;
 import org.opensearch.index.analysis.AnalysisRegistry;
-import org.opensearch.index.compositeindex.CompositeIndexSettings;
 import org.opensearch.index.remote.RemoteStorePressureService;
 import org.opensearch.index.remote.RemoteStoreStatsTrackerFactory;
 import org.opensearch.index.seqno.GlobalCheckpointSyncAction;
@@ -2079,8 +2078,7 @@ public void onFailure(final Exception e) {
                     new RemoteStoreStatsTrackerFactory(clusterService, settings),
                     DefaultRecoverySettings.INSTANCE,
                     new CacheModule(new ArrayList<>(), settings).getCacheService(),
-                    DefaultRemoteStoreSettings.INSTANCE,
-                    CompositeIndexSettings.DefaultCompositeIndexSettings.INSTANCE
+                    DefaultRemoteStoreSettings.INSTANCE
                 );
                 final RecoverySettings recoverySettings = new RecoverySettings(settings, clusterSettings);
                 snapshotShardsService = new SnapshotShardsService(