Skip to content

Commit

Permalink
addressing review comments and adding validations, integ tests
Browse files Browse the repository at this point in the history 
Signed-off-by: Bharathwaj G <[email protected]>
  • Loading branch information
@bharath-techie @sarthakaggarwal97
bharath-techie authored and sarthakaggarwal97 committed Jun 23, 2024
1 parent 6bb22ea commit 950632f
Show file tree
Hide file tree
Showing 26 changed files with 872 additions and 265 deletions.
2 changes: 1 addition & 1 deletion distribution/src/config/opensearch.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,4 @@ ${path.logs}
#
# Gates the functionality of star tree index, which improves the performance of search aggregations.
#
#opensearch.experimental.feature.composite.star_tree.enabled: true
#opensearch.experimental.feature.composite_index.star_tree.enabled: true
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1320,7 +1320,7 @@ private static void updateIndexMappingsAndBuildSortOrder(
}

if (mapperService.isCompositeIndexPresent()) {
CompositeIndexValidator.validate(mapperService, indexService.getCompositeIndexSettings());
CompositeIndexValidator.validate(mapperService, indexService.getCompositeIndexSettings(), indexService.getIndexSettings());
}

if (sourceMetadata == null) {
Expand Down
12 changes: 11 additions & 1 deletion server/src/main/java/org/opensearch/cluster/metadata/MetadataMappingService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.opensearch.core.common.Strings;
import org.opensearch.core.index.Index;
import org.opensearch.index.IndexService;
import org.opensearch.index.compositeindex.CompositeIndexValidator;
import org.opensearch.index.mapper.DocumentMapper;
import org.opensearch.index.mapper.MapperService;
import org.opensearch.index.mapper.MapperService.MergeReason;
Expand Down Expand Up @@ -282,6 +283,7 @@ private ClusterState applyRequest(
// first, simulate: just call merge and ignore the result
existingMapper.merge(newMapper.mapping(), MergeReason.MAPPING_UPDATE);
}

}
Metadata.Builder builder = Metadata.builder(metadata);
boolean updated = false;
Expand All @@ -291,7 +293,7 @@ private ClusterState applyRequest(
// we use the exact same indexService and metadata we used to validate above here to actually apply the update
final Index index = indexMetadata.getIndex();
final MapperService mapperService = indexMapperServices.get(index);

boolean isCompositeFieldPresent = !mapperService.getCompositeFieldTypes().isEmpty();
CompressedXContent existingSource = null;
DocumentMapper existingMapper = mapperService.documentMapper();
if (existingMapper != null) {
Expand All @@ -302,6 +304,14 @@ private ClusterState applyRequest(
mappingUpdateSource,
MergeReason.MAPPING_UPDATE
);

CompositeIndexValidator.validate(
mapperService,
indicesService.getCompositeIndexSettings(),
mapperService.getIndexSettings(),
isCompositeFieldPresent
);

CompressedXContent updatedSource = mergedMapper.mappingSource();

if (existingSource != null) {
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
import org.opensearch.index.SearchSlowLog;
import org.opensearch.index.TieredMergePolicyProvider;
import org.opensearch.index.cache.bitset.BitsetFilterCache;
import org.opensearch.index.compositeindex.startree.StarTreeIndexSettings;
import org.opensearch.index.compositeindex.datacube.startree.StarTreeIndexSettings;
import org.opensearch.index.engine.EngineConfig;
import org.opensearch.index.fielddata.IndexFieldDataService;
import org.opensearch.index.mapper.FieldMapper;
Expand Down Expand Up @@ -246,6 +246,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
StarTreeIndexSettings.STAR_TREE_MAX_FIELDS_SETTING,
StarTreeIndexSettings.DEFAULT_METRICS_LIST,
StarTreeIndexSettings.DEFAULT_DATE_INTERVALS,
StarTreeIndexSettings.STAR_TREE_MAX_DATE_INTERVALS_SETTING,

// validate that built-in similarities don't get redefined
Setting.groupSetting("index.similarity.", (s) -> {
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/common/util/FeatureFlags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public class FeatureFlags {
* Gates the functionality of star tree index, which improves the performance of search
* aggregations.
*/
public static final String STAR_TREE_INDEX = "opensearch.experimental.feature.composite.star_tree.enabled";
public static final String STAR_TREE_INDEX = "opensearch.experimental.feature.composite_index.star_tree.enabled";
public static final Setting<Boolean> STAR_TREE_INDEX_SETTING = Setting.boolSetting(STAR_TREE_INDEX, false, Property.NodeScope);

private static final List<Setting<Boolean>> ALL_FEATURE_FLAG_SETTINGS = List.of(
Expand Down
14 changes: 13 additions & 1 deletion server/src/main/java/org/opensearch/index/compositeindex/CompositeIndexSettings.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
@ExperimentalApi
public class CompositeIndexSettings {
public static final Setting<Boolean> STAR_TREE_INDEX_ENABLED_SETTING = Setting.boolSetting(
"indices.composite.star_tree.enabled",
"indices.composite_index.star_tree.enabled",
false,
value -> {
if (FeatureFlags.isEnabled(FeatureFlags.STAR_TREE_INDEX_SETTING) == false && value == true) {
Expand Down Expand Up @@ -52,4 +52,16 @@ 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)
);
}
}
52 changes: 49 additions & 3 deletions server/src/main/java/org/opensearch/index/compositeindex/CompositeIndexValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
package org.opensearch.index.compositeindex;

import org.opensearch.common.annotation.ExperimentalApi;
import org.opensearch.index.IndexSettings;
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;
Expand All @@ -26,12 +30,44 @@
@ExperimentalApi
public class CompositeIndexValidator {

public static void validate(MapperService mapperService, CompositeIndexSettings compositeIndexSettings) {
validateStarTreeMappings(mapperService, compositeIndexSettings);
public static void validate(MapperService mapperService, CompositeIndexSettings compositeIndexSettings, IndexSettings indexSettings) {
validateStarTreeMappings(mapperService, compositeIndexSettings, indexSettings);
}

private static void validateStarTreeMappings(MapperService mapperService, CompositeIndexSettings compositeIndexSettings) {
public static void validate(
MapperService mapperService,
CompositeIndexSettings compositeIndexSettings,
IndexSettings indexSettings,
boolean isCompositeFieldPresent
) {
if (!isCompositeFieldPresent && mapperService.isCompositeIndexPresent()) {
throw new IllegalArgumentException(
String.format(
Locale.ROOT,
"Composite fields must be specified during index creation, addition of new composite fields during update is not supported"
)
);
}
validateStarTreeMappings(mapperService, compositeIndexSettings, indexSettings);
}

private static void validateStarTreeMappings(
MapperService mapperService,
CompositeIndexSettings compositeIndexSettings,
IndexSettings indexSettings
) {
Set<CompositeMappedFieldType> compositeFieldTypes = mapperService.getCompositeFieldTypes();
if (mapperService.getCompositeFieldTypes().size() > StarTreeIndexSettings.STAR_TREE_MAX_FIELDS_SETTING.get(
indexSettings.getSettings()
)) {
throw new IllegalArgumentException(
String.format(
Locale.ROOT,
"Index cannot have more than [%s] star tree fields",
StarTreeIndexSettings.STAR_TREE_MAX_FIELDS_SETTING.get(indexSettings.getSettings())
)
);
}
for (CompositeMappedFieldType compositeFieldType : compositeFieldTypes) {
if (!(compositeFieldType instanceof StarTreeMapper.StarTreeFieldType)) {
return;
Expand All @@ -48,6 +84,11 @@ private static void validateStarTreeMappings(MapperService mapperService, Compos
CompositeDataCubeFieldType dataCubeFieldType = (CompositeDataCubeFieldType) 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()) {
throw new IllegalArgumentException(
String.format(
Expand All @@ -61,6 +102,11 @@ private static void validateStarTreeMappings(MapperService mapperService, Compos
}
for (Metric metric : dataCubeFieldType.getMetrics()) {
MappedFieldType ft = mapperService.fieldType(metric.getField());
if (ft == null) {
throw new IllegalArgumentException(
String.format(Locale.ROOT, "unknown metric field [%s] as part of star tree field", metric.getField())
);
}
if (!ft.isAggregatable()) {
throw new IllegalArgumentException(
String.format(
Expand Down
49 changes: 34 additions & 15 deletions ...h/index/compositeindex/DateDimension.java → ...ompositeindex/datacube/DateDimension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,23 @@
* compatible open source license.
*/

package org.opensearch.index.compositeindex;
package org.opensearch.index.compositeindex.datacube;

import org.opensearch.common.Rounding;
import org.opensearch.common.annotation.ExperimentalApi;
import org.opensearch.common.xcontent.support.XContentMapValues;
import org.opensearch.core.xcontent.XContentBuilder;
import org.opensearch.index.compositeindex.startree.StarTreeIndexSettings;
import org.opensearch.index.compositeindex.datacube.startree.StarTreeIndexSettings;
import org.opensearch.index.mapper.Mapper;
import org.opensearch.index.mapper.StarTreeMapper;

import java.io.IOException;
import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;

Expand All @@ -32,25 +34,26 @@
@ExperimentalApi
public class DateDimension extends Dimension {
private final List<Rounding.DateTimeUnit> calendarIntervals;
public static final String CALENDAR_INTERVALS = "calendar_intervals";
public static final String DATE = "date";

public DateDimension(String name, List<Rounding.DateTimeUnit> intervals) {
public DateDimension(String name, Map<String, Object> dimensionMap, Mapper.TypeParser.ParserContext c) {
super(name);
this.calendarIntervals = intervals;
}

@SuppressWarnings("unchecked")
public DateDimension(Map.Entry<String, Object> dimension, Mapper.TypeParser.ParserContext c) {
super(dimension.getKey());
List<String> intervalStrings = XContentMapValues.extractRawValues("calendar_interval", (Map<String, Object>) dimension.getValue())
List<String> intervalStrings = XContentMapValues.extractRawValues(CALENDAR_INTERVALS, dimensionMap)
.stream()
.map(Object::toString)
.collect(Collectors.toList());
if (intervalStrings == null || intervalStrings.isEmpty()) {
this.calendarIntervals = StarTreeIndexSettings.DEFAULT_DATE_INTERVALS.get(c.getSettings());
} else {
if (intervalStrings.size() > 3) {
if (intervalStrings.size() > StarTreeIndexSettings.STAR_TREE_MAX_DATE_INTERVALS_SETTING.get(c.getSettings())) {
throw new IllegalArgumentException(
String.format(Locale.ROOT, "At most 3 calendar intervals are allowed in dimension [%s]", dimension.getKey())
String.format(
Locale.ROOT,
"At most [%s] calendar intervals are allowed in dimension [%s]",
StarTreeIndexSettings.STAR_TREE_MAX_DATE_INTERVALS_SETTING.get(c.getSettings()),
name
)
);
}
Set<Rounding.DateTimeUnit> calendarIntervals = new LinkedHashSet<>();
Expand All @@ -59,6 +62,7 @@ public DateDimension(Map.Entry<String, Object> dimension, Mapper.TypeParser.Pars
}
this.calendarIntervals = new ArrayList<>(calendarIntervals);
}
dimensionMap.remove(CALENDAR_INTERVALS);
}

public List<Rounding.DateTimeUnit> getIntervals() {
Expand All @@ -67,14 +71,29 @@ public List<Rounding.DateTimeUnit> getIntervals() {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(this.getField());
builder.field("type", "date");
builder.startArray("calendar_interval");
builder.startObject();
builder.field(StarTreeMapper.NAME, this.getField());
builder.field(StarTreeMapper.TYPE, DATE);
builder.startArray(CALENDAR_INTERVALS);
for (Rounding.DateTimeUnit interval : calendarIntervals) {
builder.value(interval.shortName());
}
builder.endArray();
builder.endObject();
return builder;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
if (!super.equals(o)) return false;
DateDimension that = (DateDimension) o;
return Objects.equals(calendarIntervals, that.calendarIntervals);
}

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), calendarIntervals);
}
}
23 changes: 20 additions & 3 deletions ...earch/index/compositeindex/Dimension.java → ...ex/compositeindex/datacube/Dimension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@
* compatible open source license.
*/

package org.opensearch.index.compositeindex;
package org.opensearch.index.compositeindex.datacube;

import org.opensearch.common.annotation.ExperimentalApi;
import org.opensearch.core.xcontent.ToXContent;
import org.opensearch.core.xcontent.XContentBuilder;
import org.opensearch.index.mapper.StarTreeMapper;

import java.io.IOException;
import java.util.Objects;

/**
* Composite index dimension base class
Expand All @@ -21,6 +23,7 @@
*/
@ExperimentalApi
public class Dimension implements ToXContent {
public static final String NUMERIC = "numeric";
private final String field;

public Dimension(String field) {
Expand All @@ -33,9 +36,23 @@ public String getField() {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(field);
builder.field("type", "numeric");
builder.startObject();
builder.field(StarTreeMapper.NAME, field);
builder.field(StarTreeMapper.TYPE, NUMERIC);
builder.endObject();
return builder;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
Dimension dimension = (Dimension) o;
return Objects.equals(field, dimension.field);
}

@Override
public int hashCode() {
return Objects.hash(field);
}
}
Loading

0 comments on commit 950632f

Please sign in to comment.