honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[Transform] Make pivot validation sub-agg aware (#62381) 

With the addition of sub aggregations like filter, the validation could fail if 2 sub aggs use the
same output name. This change makes validation sub-agg aware.

fixes #57814
This commit is contained in:
Hendrik Muhs 2020-09-16 07:54:29 +02:00
parent a11dfbe031
commit 8566e9e3e7
2 changed files with 211 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java
View File

@ -197,9 +197,8 @@ public class PivotConfig implements Writeable, ToXContentObject {
return Collections.emptyList(); return Collections.emptyList();
} }
List<String> usedNames = new ArrayList<>(); List<String> usedNames = new ArrayList<>();
// TODO this will need to change once we allow multi-bucket aggs + field merging aggregationConfig.getAggregatorFactories().forEach(agg -> addAggNames("", agg, usedNames));
aggregationConfig.getAggregatorFactories().forEach(agg -> addAggNames(agg, usedNames)); aggregationConfig.getPipelineAggregatorFactories().forEach(agg -> addAggNames("", agg, usedNames));
aggregationConfig.getPipelineAggregatorFactories().forEach(agg -> addAggNames(agg, usedNames));
usedNames.addAll(groups.getGroups().keySet()); usedNames.addAll(groups.getGroups().keySet());
return aggFieldValidation(usedNames); return aggFieldValidation(usedNames);
} }
@ -251,13 +250,18 @@ public class PivotConfig implements Writeable, ToXContentObject {
return validationFailures; return validationFailures;
} }
private static void addAggNames(AggregationBuilder aggregationBuilder, Collection<String> names) { private static void addAggNames(String namePrefix, AggregationBuilder aggregationBuilder, Collection<String> names) {
names.add(aggregationBuilder.getName()); if (aggregationBuilder.getSubAggregations().isEmpty() && aggregationBuilder.getPipelineAggregations().isEmpty()) {
aggregationBuilder.getSubAggregations().forEach(agg -> addAggNames(agg, names)); names.add(namePrefix + aggregationBuilder.getName());
aggregationBuilder.getPipelineAggregations().forEach(agg -> addAggNames(agg, names)); return;
} }
private static void addAggNames(PipelineAggregationBuilder pipelineAggregationBuilder, Collection<String> names) { String newNamePrefix = namePrefix + aggregationBuilder.getName() + ".";
names.add(pipelineAggregationBuilder.getName()); aggregationBuilder.getSubAggregations().forEach(agg -> addAggNames(newNamePrefix, agg, names));
aggregationBuilder.getPipelineAggregations().forEach(agg -> addAggNames(newNamePrefix, agg, names));
}
private static void addAggNames(String namePrefix, PipelineAggregationBuilder pipelineAggregationBuilder, Collection<String> names) {
names.add(namePrefix + pipelineAggregationBuilder.getName());
} }
} }

198
x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java
View File

@ -158,6 +158,28 @@ public class PivotConfigTests extends AbstractSerializingTransformTestCase<Pivot
expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false)); expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false));
} }
public void testAggDuplicates() throws IOException {
String pivot = "{"
+ " \"group_by\": {"
+ " \"id\": {"
+ " \"terms\": {"
+ " \"field\": \"id\""
+ "} } },"
+ " \"aggs\": {"
+ " \"points\": {"
+ " \"max\": {"
+ " \"field\": \"points\""
+ "} },"
+ " \"points\": {"
+ " \"min\": {"
+ " \"field\": \"points\""
+ "} } }"
+ "}";
// this throws early in the agg framework
expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false));
}
public void testValidAggNames() throws IOException { public void testValidAggNames() throws IOException {
String pivotAggs = "{" String pivotAggs = "{"
+ " \"group_by\": {" + " \"group_by\": {"
@ -176,6 +198,182 @@ public class PivotConfigTests extends AbstractSerializingTransformTestCase<Pivot
assertTrue(fieldValidation.isEmpty()); assertTrue(fieldValidation.isEmpty());
} }
public void testValidAggNamesNested() throws IOException {
String pivotAggs = "{"
+ "\"group_by\": {"
+ " \"timestamp\": {"
+ " \"date_histogram\": {"
+ " \"field\": \"timestamp\","
+ " \"fixed_interval\": \"1d\""
+ " }"
+ " }"
+ "},"
+ "\"aggregations\": {"
+ " \"jp\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.src\": \"JP\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"os.dc\": {"
+ " \"cardinality\": {"
+ " \"field\": \"machine.os.keyword\""
+ " }"
+ " }"
+ " }"
+ " },"
+ " \"us\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.src\": \"US\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"os.dc\": {"
+ " \"cardinality\": {"
+ " \"field\": \"machine.os.keyword\""
+ "} } } } } }";
PivotConfig pivotConfig = createPivotConfigFromString(pivotAggs, true);
assertTrue(pivotConfig.isValid());
List<String> fieldValidation = pivotConfig.aggFieldValidation();
assertTrue(Strings.collectionToCommaDelimitedString(fieldValidation), fieldValidation.isEmpty());
}
public void testValidAggNamesNestedTwice() throws IOException {
String pivotAggs = "{"
+ " \"group_by\": {"
+ " \"timestamp\": {"
+ " \"date_histogram\": {"
+ " \"field\": \"timestamp\","
+ " \"fixed_interval\": \"1d\""
+ " }"
+ " }"
+ " },"
+ " \"aggregations\": {"
+ " \"jp\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.src\": \"JP\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"us\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.dest\": \"US\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"os.dc\": {"
+ " \"cardinality\": {"
+ " \"field\": \"machine.os.keyword\""
+ " }"
+ " }"
+ " }"
+ " }"
+ " }"
+ " },"
+ " \"us\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.src\": \"US\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"jp\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.dest\": \"JP\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"os.dc\": {"
+ " \"cardinality\": {"
+ " \"field\": \"machine.os.keyword\""
+ " }"
+ " }"
+ " }"
+ " }"
+ " }"
+ " }"
+ " }"
+ " }";
PivotConfig pivotConfig = createPivotConfigFromString(pivotAggs, true);
assertTrue(pivotConfig.isValid());
List<String> fieldValidation = pivotConfig.aggFieldValidation();
assertTrue(Strings.collectionToCommaDelimitedString(fieldValidation), fieldValidation.isEmpty());
}
public void testInValidAggNamesNestedTwice() throws IOException {
String pivotAggs = "{"
+ " \"group_by\": {"
+ " \"jp.us.os.dc\": {"
+ " \"date_histogram\": {"
+ " \"field\": \"timestamp\","
+ " \"fixed_interval\": \"1d\""
+ " }"
+ " }"
+ " },"
+ " \"aggregations\": {"
+ " \"jp\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.src\": \"JP\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"us\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.dest\": \"US\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"os.dc\": {"
+ " \"cardinality\": {"
+ " \"field\": \"machine.os.keyword\""
+ " }"
+ " }"
+ " }"
+ " }"
+ " }"
+ " },"
+ " \"us\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.src\": \"US\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"jp\": {"
+ " \"filter\": {"
+ " \"term\": {"
+ " \"geo.dest\": \"JP\""
+ " }"
+ " },"
+ " \"aggs\": {"
+ " \"os.dc\": {"
+ " \"cardinality\": {"
+ " \"field\": \"machine.os.keyword\""
+ " }"
+ " }"
+ " }"
+ " }"
+ " }"
+ " }"
+ " }"
+ " }";
PivotConfig pivotConfig = createPivotConfigFromString(pivotAggs, true);
assertTrue(pivotConfig.isValid());
List<String> fieldValidation = pivotConfig.aggFieldValidation();
assertThat(fieldValidation, containsInAnyOrder("duplicate field [jp.us.os.dc] detected"));
}
public void testAggNameValidationsWithoutIssues() { public void testAggNameValidationsWithoutIssues() {
String prefix = randomAlphaOfLength(10) + "1"; String prefix = randomAlphaOfLength(10) + "1";
String prefix2 = randomAlphaOfLength(10) + "2"; String prefix2 = randomAlphaOfLength(10) + "2";