Skip to content

[ML] Disable dynamic mapping to DFA dest index #65972

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 8, 2020
Merged

[ML] Disable dynamic mapping to DFA dest index #65972

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39205f1
[ML] Disable dynamic mapping to DFA dest index
dimitris-athanasiou Dec 7, 2020
f5e061e
Improve map assertion
dimitris-athanasiou Dec 7, 2020
53a0a29
Add assertion for is_training in regression tests
dimitris-athanasiou Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions ...core/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/Classification.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.mapper.BooleanFieldMapper;
import org.elasticsearch.index.mapper.KeywordFieldMapper;
import org.elasticsearch.index.mapper.NumberFieldMapper;
import org.elasticsearch.index.mapper.ObjectMapper;
Expand Down Expand Up @@ -139,7 +140,6 @@ public static Classification fromXContent(XContentParser parser, boolean ignoreU

Map<String, Object> properties = new HashMap<>();
properties.put("feature_name", Collections.singletonMap("type", KeywordFieldMapper.CONTENT_TYPE));
properties.put("importance", Collections.singletonMap("type", NumberFieldMapper.NumberType.DOUBLE.typeName()));
properties.put("classes", classesMapping);

Map<String, Object> mapping = new HashMap<>();
Expand Down Expand Up @@ -374,15 +374,18 @@ public List<FieldCardinalityConstraint> getFieldCardinalityConstraints() {

@SuppressWarnings("unchecked")
@Override
public Map<String, Object> getExplicitlyMappedFields(String resultsFieldName, FieldCapabilitiesResponse fieldCapabilitiesResponse) {
public Map<String, Object> getResultMappings(String resultsFieldName, FieldCapabilitiesResponse fieldCapabilitiesResponse) {
Map<String, Object> additionalProperties = new HashMap<>();
additionalProperties.put(resultsFieldName + ".is_training", Collections.singletonMap("type", BooleanFieldMapper.CONTENT_TYPE));
additionalProperties.put(resultsFieldName + ".prediction_probability",
Collections.singletonMap("type", NumberFieldMapper.NumberType.DOUBLE.typeName()));
additionalProperties.put(resultsFieldName + ".prediction_score",
Collections.singletonMap("type", NumberFieldMapper.NumberType.DOUBLE.typeName()));
additionalProperties.put(resultsFieldName + ".feature_importance", FEATURE_IMPORTANCE_MAPPING);
if (fieldCapabilitiesResponse == null) {
return additionalProperties;
}

Map<String, FieldCapabilities> dependentVariableFieldCaps = fieldCapabilitiesResponse.getField(dependentVariable);
if (dependentVariableFieldCaps == null || dependentVariableFieldCaps.isEmpty()) {
return additionalProperties;
throw ExceptionsHelper.badRequestException("no mappings could be found for required field [{}]", DEPENDENT_VARIABLE);
}
Object dependentVariableMappingType = dependentVariableFieldCaps.values().iterator().next().getType();
additionalProperties.put(
Expand All @@ -391,6 +394,7 @@ public Map<String, Object> getExplicitlyMappedFields(String resultsFieldName, Fi
Map<String, Object> topClassesProperties = new HashMap<>();
topClassesProperties.put("class_name", Collections.singletonMap("type", dependentVariableMappingType));
topClassesProperties.put("class_probability", Collections.singletonMap("type", NumberFieldMapper.NumberType.DOUBLE.typeName()));
topClassesProperties.put("class_score", Collections.singletonMap("type", NumberFieldMapper.NumberType.DOUBLE.typeName()));

Map<String, Object> topClassesMapping = new HashMap<>();
topClassesMapping.put("type", ObjectMapper.NESTED_CONTENT_TYPE);
Expand Down
2 changes: 1 addition & 1 deletion ...e/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/DataFrameAnalysis.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public interface DataFrameAnalysis extends ToXContentObject, NamedWriteable {
* @param fieldCapabilitiesResponse field capabilities fetched for this analysis' required fields
* @return {@link Map} containing fields for which the mappings should be handled explicitly
*/
Map<String, Object> getExplicitlyMappedFields(String resultsFieldName, FieldCapabilitiesResponse fieldCapabilitiesResponse);
Map<String, Object> getResultMappings(String resultsFieldName, FieldCapabilitiesResponse fieldCapabilitiesResponse);

/**
* @return {@code true} if this analysis supports data frame rows with missing values
Expand Down
2 changes: 1 addition & 1 deletion ...re/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/OutlierDetection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ public List<FieldCardinalityConstraint> getFieldCardinalityConstraints() {
}

@Override
public Map<String, Object> getExplicitlyMappedFields(String resultsFieldName, FieldCapabilitiesResponse fieldCapabilitiesResponse) {
public Map<String, Object> getResultMappings(String resultsFieldName, FieldCapabilitiesResponse fieldCapabilitiesResponse) {
Map<String, Object> additionalProperties = new HashMap<>();
additionalProperties.put(resultsFieldName + ".outlier_score",
Collections.singletonMap("type", NumberFieldMapper.NumberType.DOUBLE.typeName()));
Expand Down
4 changes: 3 additions & 1 deletion ...gin/core/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/Regression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.mapper.BooleanFieldMapper;
import org.elasticsearch.index.mapper.KeywordFieldMapper;
import org.elasticsearch.index.mapper.NumberFieldMapper;
import org.elasticsearch.index.mapper.ObjectMapper;
Expand Down Expand Up @@ -285,8 +286,9 @@ public List<FieldCardinalityConstraint> getFieldCardinalityConstraints() {
}

@Override
public Map<String, Object> getExplicitlyMappedFields(String resultsFieldName, FieldCapabilitiesResponse fieldCapabilitiesResponse) {
public Map<String, Object> getResultMappings(String resultsFieldName, FieldCapabilitiesResponse fieldCapabilitiesResponse) {
Map<String, Object> additionalProperties = new HashMap<>();
additionalProperties.put(resultsFieldName + ".is_training", Collections.singletonMap("type", BooleanFieldMapper.CONTENT_TYPE));
additionalProperties.put(resultsFieldName + ".feature_importance", FEATURE_IMPORTANCE_MAPPING);
// Prediction field should be always mapped as "double" rather than "float" in order to increase precision in case of
// high (over 10M) values of dependent variable.
Expand Down
37 changes: 18 additions & 19 deletions ...src/test/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/ClassificationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,43 +362,42 @@ public void testFieldCardinalityLimitsIsNonEmpty() {
assertThat(constraints.get(0).getUpperBound(), equalTo(30L));
}

public void testGetExplicitlyMappedFields_FieldCapabilitiesResponseIsNull() {
Map<String, Object> explicitlyMappedFields = new Classification("foo").getExplicitlyMappedFields("results", null);
assertThat(explicitlyMappedFields, equalTo(singletonMap("results.feature_importance", Classification.FEATURE_IMPORTANCE_MAPPING)));
}

public void testGetExplicitlyMappedFields_DependentVariableMappingIsAbsent() {
public void testGetResultMappings_DependentVariableMappingIsAbsent() {
FieldCapabilitiesResponse fieldCapabilitiesResponse = new FieldCapabilitiesResponse(new String[0], Collections.emptyMap());
Map<String, Object> explicitlyMappedFields =
new Classification("foo").getExplicitlyMappedFields("results", fieldCapabilitiesResponse);
assertThat(explicitlyMappedFields, equalTo(singletonMap("results.feature_importance", Classification.FEATURE_IMPORTANCE_MAPPING)));
expectThrows(ElasticsearchStatusException.class,
() -> new Classification("foo").getResultMappings("results", fieldCapabilitiesResponse));
}

public void testGetExplicitlyMappedFields_DependentVariableMappingHasNoTypes() {
public void testGetResultMappings_DependentVariableMappingHasNoTypes() {
FieldCapabilitiesResponse fieldCapabilitiesResponse =
new FieldCapabilitiesResponse(new String[0], Collections.singletonMap("foo", Collections.emptyMap()));
Map<String, Object> explicitlyMappedFields =
new Classification("foo").getExplicitlyMappedFields("results", fieldCapabilitiesResponse);
assertThat(explicitlyMappedFields, equalTo(singletonMap("results.feature_importance", Classification.FEATURE_IMPORTANCE_MAPPING)));
expectThrows(ElasticsearchStatusException.class,
() -> new Classification("foo").getResultMappings("results", fieldCapabilitiesResponse));
}

public void testGetExplicitlyMappedFields_DependentVariableMappingIsPresent() {
public void testGetResultMappings_DependentVariableMappingIsPresent() {
Map<String, Object> expectedTopClassesMapping = new HashMap<>() {{
put("type", "nested");
put("properties", new HashMap<>() {{
put("class_name", singletonMap("type", "dummy"));
put("class_probability", singletonMap("type", "double"));
put("class_score", singletonMap("type", "double"));
}});
}};
FieldCapabilitiesResponse fieldCapabilitiesResponse =
new FieldCapabilitiesResponse(
new String[0],
Collections.singletonMap("foo", Collections.singletonMap("dummy", createFieldCapabilities("foo", "dummy"))));
Map<String, Object> explicitlyMappedFields =
new Classification("foo").getExplicitlyMappedFields("results", fieldCapabilitiesResponse);
assertThat(explicitlyMappedFields, hasEntry("results.foo_prediction", singletonMap("type", "dummy")));
assertThat(explicitlyMappedFields, hasEntry("results.top_classes", expectedTopClassesMapping));
assertThat(explicitlyMappedFields, hasEntry("results.feature_importance", Classification.FEATURE_IMPORTANCE_MAPPING));

Map<String, Object> resultMappings =
new Classification("foo").getResultMappings("results", fieldCapabilitiesResponse);

assertThat(resultMappings, hasEntry("results.foo_prediction", singletonMap("type", "dummy")));
Copy link
Contributor

@przemekwitek przemekwitek Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional] An alternative which I sometimes find more readable is:

assertThat(resultMappings, allOf(hasEntry(...), ..., hasEntry(...)));

assertThat(resultMappings, hasEntry("results.prediction_probability", singletonMap("type", "double")));
assertThat(resultMappings, hasEntry("results.prediction_score", singletonMap("type", "double")));
assertThat(resultMappings, hasEntry("results.is_training", singletonMap("type", "boolean")));
assertThat(resultMappings, hasEntry("results.top_classes", expectedTopClassesMapping));
assertThat(resultMappings, hasEntry("results.feature_importance", Classification.FEATURE_IMPORTANCE_MAPPING));
}

public void testToXContent_GivenVersionBeforeRandomizeSeedWasIntroduced() throws IOException {
Expand Down
4 changes: 2 additions & 2 deletions ...c/test/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/OutlierDetectionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ public void testFieldCardinalityLimitsIsEmpty() {
assertThat(createTestInstance().getFieldCardinalityConstraints(), is(empty()));
}

public void testGetExplicitlyMappedFields() {
Map<String, Object> mappedFields = createTestInstance().getExplicitlyMappedFields("test", null);
public void testGetResultMappings() {
Map<String, Object> mappedFields = createTestInstance().getResultMappings("test", null);
assertThat(mappedFields.size(), equalTo(2));
assertThat(mappedFields, hasKey("test.outlier_score"));
assertThat(mappedFields.get("test.outlier_score"),
Expand Down
8 changes: 4 additions & 4 deletions ...ore/src/test/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/RegressionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,10 +321,10 @@ public void testFieldCardinalityLimitsIsEmpty() {
assertThat(createTestInstance().getFieldCardinalityConstraints(), is(empty()));
}

public void testGetExplicitlyMappedFields() {
Map<String, Object> explicitlyMappedFields = new Regression("foo").getExplicitlyMappedFields("results", null);
assertThat(explicitlyMappedFields, hasEntry("results.foo_prediction", Collections.singletonMap("type", "double")));
assertThat(explicitlyMappedFields, hasEntry("results.feature_importance", Regression.FEATURE_IMPORTANCE_MAPPING));
public void testGetResultMappings() {
Map<String, Object> resultMappings = new Regression("foo").getResultMappings("results", null);
assertThat(resultMappings, hasEntry("results.foo_prediction", Collections.singletonMap("type", "double")));
assertThat(resultMappings, hasEntry("results.feature_importance", Regression.FEATURE_IMPORTANCE_MAPPING));
Copy link
Contributor

@przemekwitek przemekwitek Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add is_training here?

}

public void testGetStateDocId() {
Expand Down
10 changes: 3 additions & 7 deletions x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/dataframe/DestinationIndex.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ private static CreateIndexRequest createIndexRequest(Clock clock,
Map<String, Object> mappingsAsMap = mappings.sourceAsMap();
Map<String, Object> properties = getOrPutDefault(mappingsAsMap, PROPERTIES, HashMap::new);
checkResultsFieldIsNotPresentInProperties(config, properties);
properties.putAll(createAdditionalMappings(config, Collections.unmodifiableMap(properties), fieldCapabilitiesResponse));
properties.putAll(createAdditionalMappings(config, fieldCapabilitiesResponse));
Map<String, Object> metadata = getOrPutDefault(mappingsAsMap, META, HashMap::new);
metadata.putAll(createMetadata(config.getId(), clock, Version.CURRENT));
return new CreateIndexRequest(destinationIndex, settings).mapping(mappingsAsMap);
Expand Down Expand Up @@ -210,12 +210,11 @@ private static Integer findMaxSettingValue(GetSettingsResponse settingsResponse,
}

private static Map<String, Object> createAdditionalMappings(DataFrameAnalyticsConfig config,
Map<String, Object> mappingsProperties,
FieldCapabilitiesResponse fieldCapabilitiesResponse) {
Map<String, Object> properties = new HashMap<>();
properties.put(INCREMENTAL_ID, Map.of("type", NumberFieldMapper.NumberType.LONG.typeName()));
properties.putAll(
config.getAnalysis().getExplicitlyMappedFields(
config.getAnalysis().getResultMappings(
config.getDest().getResultsField(), fieldCapabilitiesResponse));
return properties;
}
Expand Down Expand Up @@ -259,10 +258,7 @@ public static void updateMappingsToDestIndex(Client client,
ActionListener<FieldCapabilitiesResponse> fieldCapabilitiesListener = ActionListener.wrap(
fieldCapabilitiesResponse -> {
// Determine mappings to be added to the destination index
Map<String, Object> addedMappings =
Map.of(
PROPERTIES,
createAdditionalMappings(config, Collections.unmodifiableMap(destPropertiesAsMap), fieldCapabilitiesResponse));
Map<String, Object> addedMappings = Map.of(PROPERTIES, createAdditionalMappings(config, fieldCapabilitiesResponse));

// Add the mappings to the destination index
PutMappingRequest putMappingRequest =
Expand Down
8 changes: 5 additions & 3 deletions x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/dataframe/MappingsMerger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import org.elasticsearch.xpack.core.ml.dataframe.DataFrameAnalyticsSource;
import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper;

import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
Expand Down Expand Up @@ -89,8 +88,11 @@ static MappingMetadata mergeMappings(DataFrameAnalyticsSource source,
mergedMappings.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().mapping)));
}

private static MappingMetadata createMappingMetadata(String type, Map<String, Object> mappings) {
return new MappingMetadata(type, Collections.singletonMap("properties", mappings));
private static MappingMetadata createMappingMetadata(String type, Map<String, Object> properties) {
Map<String, Object> mappings = new HashMap<>();
mappings.put("dynamic", false);
mappings.put("properties", properties);
return new MappingMetadata(type, mappings);
}

private static class IndexAndMapping {
Expand Down
11 changes: 9 additions & 2 deletions x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/dataframe/MappingsMergerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@ public void testMergeMappings_GivenIndicesWithIdenticalMappings() {

MappingMetadata mergedMappings = MappingsMerger.mergeMappings(newSource(), getMappingsResponse);

assertThat(mergedMappings.getSourceAsMap(), equalTo(index1Mappings));
Map<String, Object> mergedMappingsMap = mergedMappings.getSourceAsMap();
assertThat(mergedMappingsMap.size(), equalTo(2));
assertThat(mergedMappingsMap.containsKey("dynamic"), is(true));
assertThat(mergedMappingsMap.get("dynamic"), equalTo(false));
assertThat(mergedMappingsMap.containsKey("properties"), is(true));
assertThat(mergedMappingsMap.get("properties"), equalTo(index1Mappings.get("properties")));
Copy link

@droberts195 droberts195 Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you constructed a map containing the expected contents, then asserted equality with the merged mappings map then you'd get better feedback in the case of failure - both maps would get printed instead of (for example) just finding out that there were 3 items instead of 2.

}

public void testMergeMappings_GivenFieldWithDifferentMapping() {
Expand Down Expand Up @@ -80,7 +85,9 @@ public void testMergeMappings_GivenIndicesWithDifferentMappingsButNoConflicts()
MappingMetadata mergedMappings = MappingsMerger.mergeMappings(newSource(), getMappingsResponse);

Map<String, Object> mappingsAsMap = mergedMappings.getSourceAsMap();
assertThat(mappingsAsMap.size(), equalTo(1));
assertThat(mappingsAsMap.size(), equalTo(2));
assertThat(mappingsAsMap.containsKey("dynamic"), is(true));
assertThat(mappingsAsMap.get("dynamic"), equalTo(false));
assertThat(mappingsAsMap.containsKey("properties"), is(true));

@SuppressWarnings("unchecked")
Expand Down