From 8566e9e3e7b6e96fd1b19cb788755a57550bebfd Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Wed, 16 Sep 2020 07:54:29 +0200 Subject: [PATCH] [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 --- .../transforms/pivot/PivotConfig.java | 22 +- .../transforms/pivot/PivotConfigTests.java | 198 ++++++++++++++++++ 2 files changed, 211 insertions(+), 9 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java index e5225cb4ab9..c4e8636f155 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java @@ -197,9 +197,8 @@ public class PivotConfig implements Writeable, ToXContentObject { return Collections.emptyList(); } List 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.getPipelineAggregatorFactories().forEach(agg -> addAggNames(agg, usedNames)); + aggregationConfig.getAggregatorFactories().forEach(agg -> addAggNames("", agg, usedNames)); + aggregationConfig.getPipelineAggregatorFactories().forEach(agg -> addAggNames("", agg, usedNames)); usedNames.addAll(groups.getGroups().keySet()); return aggFieldValidation(usedNames); } @@ -251,13 +250,18 @@ public class PivotConfig implements Writeable, ToXContentObject { return validationFailures; } - private static void addAggNames(AggregationBuilder aggregationBuilder, Collection names) { - names.add(aggregationBuilder.getName()); - aggregationBuilder.getSubAggregations().forEach(agg -> addAggNames(agg, names)); - aggregationBuilder.getPipelineAggregations().forEach(agg -> addAggNames(agg, names)); + private static void addAggNames(String namePrefix, AggregationBuilder aggregationBuilder, Collection names) { + if (aggregationBuilder.getSubAggregations().isEmpty() && aggregationBuilder.getPipelineAggregations().isEmpty()) { + names.add(namePrefix + aggregationBuilder.getName()); + return; + } + + String newNamePrefix = namePrefix + aggregationBuilder.getName() + "."; + aggregationBuilder.getSubAggregations().forEach(agg -> addAggNames(newNamePrefix, agg, names)); + aggregationBuilder.getPipelineAggregations().forEach(agg -> addAggNames(newNamePrefix, agg, names)); } - private static void addAggNames(PipelineAggregationBuilder pipelineAggregationBuilder, Collection names) { - names.add(pipelineAggregationBuilder.getName()); + private static void addAggNames(String namePrefix, PipelineAggregationBuilder pipelineAggregationBuilder, Collection names) { + names.add(namePrefix + pipelineAggregationBuilder.getName()); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java index fa69ba55638..2d97410ce45 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfigTests.java @@ -158,6 +158,28 @@ public class PivotConfigTests extends AbstractSerializingTransformTestCase 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 { String pivotAggs = "{" + " \"group_by\": {" @@ -176,6 +198,182 @@ public class PivotConfigTests extends AbstractSerializingTransformTestCase 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 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 fieldValidation = pivotConfig.aggFieldValidation(); + + assertThat(fieldValidation, containsInAnyOrder("duplicate field [jp.us.os.dc] detected")); + } + public void testAggNameValidationsWithoutIssues() { String prefix = randomAlphaOfLength(10) + "1"; String prefix2 = randomAlphaOfLength(10) + "2";