From 4b6b5dd2149e098a36b9f2782a3da342c5524bdb Mon Sep 17 00:00:00 2001 From: Benjamin Trent <4357155+benwtrent@users.noreply.github.com> Date: Mon, 6 May 2019 15:18:47 -0500 Subject: [PATCH 1/4] [ML] verify that there are no duplicate leaf fields in aggs --- .../action/PutDataFrameTransformAction.java | 8 +- .../transforms/pivot/PivotConfig.java | 69 +++++++++++++++++ .../transforms/pivot/PivotConfigTests.java | 75 ++++++++++++++++++- .../test/data_frame/transforms_crud.yml | 32 ++++++++ 4 files changed, 181 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/action/PutDataFrameTransformAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/action/PutDataFrameTransformAction.java index 0f6cc63f98851..059bad3494c07 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/action/PutDataFrameTransformAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/action/PutDataFrameTransformAction.java @@ -20,6 +20,8 @@ import java.io.IOException; import java.util.Objects; +import static org.elasticsearch.action.ValidateActions.addValidationError; + public class PutDataFrameTransformAction extends Action { public static final PutDataFrameTransformAction INSTANCE = new PutDataFrameTransformAction(); @@ -53,7 +55,11 @@ public static Request fromXContent(final XContentParser parser, final String id) @Override public ActionRequestValidationException validate() { - return null; + ActionRequestValidationException validationException = null; + for(String failure : config.getPivotConfig().aggFieldValidation()) { + validationException = addValidationError(failure, validationException); + } + return validationException; } @Override diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java index c1c894e2971ae..1a6bee7db9372 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java @@ -13,13 +13,20 @@ import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.search.aggregations.AggregationBuilder; +import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.composite.CompositeAggregationBuilder; import org.elasticsearch.xpack.core.dataframe.DataFrameField; import org.elasticsearch.xpack.core.dataframe.utils.ExceptionsHelper; import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; import java.util.Map.Entry; import java.util.Objects; +import java.util.Set; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg; @@ -144,4 +151,66 @@ public boolean isValid() { public static PivotConfig fromXContent(final XContentParser parser, boolean lenient) throws IOException { return lenient ? LENIENT_PARSER.apply(parser, null) : STRICT_PARSER.apply(parser, null); } + + /** + * Does the following checks: + * + * - determines if there are any full duplicate names between the aggregation names and the group by names. + * - finds if there are conflicting name paths that could cause a failure later when the config is started. + * + * Examples showing conflicting field name paths: + * + * aggName1: foo.bar.baz + * aggName2: foo.bar + * + * This should fail as aggName1 will cause foo.bar to be an object, causing a conflict with the use of foo.bar in aggName2. + * + * @return List of validation failure messages + */ + public List aggFieldValidation() { + if ((aggregationConfig.isValid() && groups.isValid()) == false) { + return Collections.emptyList(); + } + + List validationFailures = new ArrayList<>(); + List usedNames = new ArrayList<>(); + // TODO this will need changed once we allow multi-bucket aggs + field merging + aggregationConfig.getAggregatorFactories().forEach(agg -> addAggNames(agg, usedNames)); + aggregationConfig.getPipelineAggregatorFactories().forEach(agg -> addAggNames(agg, usedNames)); + usedNames.addAll(groups.getGroups().keySet()); + + Set leafNames = new HashSet<>(usedNames.size()); + for(String name : usedNames) { + if (leafNames.contains(name)) { + validationFailures.add("duplicate field [" + name + "] detected"); + } + leafNames.add(name); + } + + for (String fullName : usedNames) { + String[] tokens = fullName.split("\\."); + for (int i = tokens.length - 1; i > 0; i--) { + StringBuilder prefix = new StringBuilder(); + for (int j = 0; j < i; j++) { + prefix.append(tokens[j]); + } + String prefixString = prefix.toString(); + if (leafNames.contains(prefixString)) { + validationFailures.add("field [" + prefixString + "] cannot be both an object and a field"); + } + } + } + return validationFailures; + } + + + private static void addAggNames(AggregationBuilder aggregationBuilder, List names) { + names.add(aggregationBuilder.getName()); + aggregationBuilder.getSubAggregations().forEach(agg -> addAggNames(agg, names)); + aggregationBuilder.getPipelineAggregations().forEach(agg -> addAggNames(agg, names)); + } + + private static void addAggNames(PipelineAggregationBuilder pipelineAggregationBuilder, List names) { + names.add(pipelineAggregationBuilder.getName()); + } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java index 1586ea540f4b4..1de135bff6f91 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java @@ -13,6 +13,9 @@ import org.elasticsearch.xpack.core.dataframe.transforms.AbstractSerializingDataFrameTestCase; import java.io.IOException; +import java.util.List; + +import static org.hamcrest.CoreMatchers.equalTo; public class PivotConfigTests extends AbstractSerializingDataFrameTestCase { @@ -103,7 +106,7 @@ public void testEmptyGroupBy() throws IOException { assertFalse(pivotConfig.isValid()); } - public void testMissingGroupBy() throws IOException { + public void testMissingGroupBy() { String pivot = "{" + " \"aggs\": {" + " \"avg\": {" @@ -114,7 +117,7 @@ public void testMissingGroupBy() throws IOException { expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false)); } - public void testDoubleAggs() throws IOException { + public void testDoubleAggs() { String pivot = "{" + " \"group_by\": {" + " \"id\": {" @@ -136,6 +139,74 @@ public void testDoubleAggs() throws IOException { expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false)); } + public void testAggNameValidations() throws IOException { + String pivotAggs = "{" + + " \"group_by\": {" + + " \"user\": {" + + " \"terms\": {" + + " \"field\": \"id\"" + + "} } }," + + " \"aggs\": {" + + " \"user.avg\": {" + + " \"avg\": {" + + " \"field\": \"points\"" + + "} } } }"; + PivotConfig pivotConfig = createPivotConfigFromString(pivotAggs, true); + assertTrue(pivotConfig.isValid()); + List fieldValidation = pivotConfig.aggFieldValidation(); + assertFalse(fieldValidation.isEmpty()); + assertThat(fieldValidation.get(0), equalTo("field [user] cannot be both an object and a field")); + + pivotAggs = "{" + + " \"group_by\": {" + + " \"user\": {" + + " \"terms\": {" + + " \"field\": \"id\"" + + "} } }," + + " \"aggs\": {" + + " \"user\": {" + + " \"avg\": {" + + " \"field\": \"points\"" + + "} } } }"; + pivotConfig = createPivotConfigFromString(pivotAggs, true); + assertTrue(pivotConfig.isValid()); + fieldValidation = pivotConfig.aggFieldValidation(); + assertFalse(fieldValidation.isEmpty()); + assertThat(fieldValidation.get(0), equalTo("duplicate field [user] detected")); + + pivotAggs = "{" + + " \"group_by\": {" + + " \"user\": {" + + " \"terms\": {" + + " \"field\": \"id\"" + + "} } }," + + " \"aggs\": {" + + " \"avg\": {" + + " \"avg\": {" + + " \"field\": \"points\"" + + "} } } }"; + pivotConfig = createPivotConfigFromString(pivotAggs, true); + assertTrue(pivotConfig.isValid()); + fieldValidation = pivotConfig.aggFieldValidation(); + assertTrue(fieldValidation.isEmpty()); + + pivotAggs = "{" + + " \"group_by\": {" + + " \"user.id.field\": {" + + " \"terms\": {" + + " \"field\": \"id\"" + + "} } }," + + " \"aggs\": {" + + " \"avg.field.value\": {" + + " \"avg\": {" + + " \"field\": \"points\"" + + "} } } }"; + pivotConfig = createPivotConfigFromString(pivotAggs, true); + assertTrue(pivotConfig.isValid()); + fieldValidation = pivotConfig.aggFieldValidation(); + assertTrue(fieldValidation.isEmpty()); + } + private PivotConfig createPivotConfigFromString(String json, boolean lenient) throws IOException { final XContentParser parser = XContentType.JSON.xContent().createParser(xContentRegistry(), DeprecationHandler.THROW_UNSUPPORTED_OPERATION, json); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/data_frame/transforms_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/data_frame/transforms_crud.yml index fa608cefd1eb3..40af091a91bd9 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/data_frame/transforms_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/data_frame/transforms_crud.yml @@ -302,3 +302,35 @@ setup: "aggs": {"avg_response": {"avg": {"field": "responsetime"}}} } } +--- +"Test creation failures due to duplicate and conflicting field names": + - do: + catch: /duplicate field \[airline\] detected/ + data_frame.put_data_frame_transform: + transform_id: "duplicate-field-transform" + body: > + { + "source": { + "index": "source-index" + }, + "dest": { "index": "dest-index" }, + "pivot": { + "group_by": { "airline": {"terms": {"field": "airline"}}}, + "aggs": {"airline": {"avg": {"field": "responsetime"}}} + } + } + - do: + catch: /field \[airline\] cannot be both an object and a field/ + data_frame.put_data_frame_transform: + transform_id: "duplicate-field-transform" + body: > + { + "source": { + "index": "source-index" + }, + "dest": { "index": "dest-index" }, + "pivot": { + "group_by": { "airline": {"terms": {"field": "airline"}}}, + "aggs": {"airline.responsetime": {"avg": {"field": "responsetime"}}} + } + } From 639542398d5e04fc8876e91090fcd59561e75a9f Mon Sep 17 00:00:00 2001 From: Benjamin Trent <4357155+benwtrent@users.noreply.github.com> Date: Wed, 8 May 2019 10:58:27 -0500 Subject: [PATCH 2/4] addressing pr comments --- .../transforms/pivot/PivotConfig.java | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java index 1a6bee7db9372..8f4168ad3922a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -186,31 +187,23 @@ public List aggFieldValidation() { } leafNames.add(name); } - - for (String fullName : usedNames) { - String[] tokens = fullName.split("\\."); - for (int i = tokens.length - 1; i > 0; i--) { - StringBuilder prefix = new StringBuilder(); - for (int j = 0; j < i; j++) { - prefix.append(tokens[j]); - } - String prefixString = prefix.toString(); - if (leafNames.contains(prefixString)) { - validationFailures.add("field [" + prefixString + "] cannot be both an object and a field"); - } + usedNames.sort(String::compareTo); + for (int i = 0; i < usedNames.size() - 1; i++) { + if (usedNames.get(i+1).startsWith(usedNames.get(i) + ".")) { + validationFailures.add("field [" + usedNames.get(i) + "] cannot be both an object and a field"); } } return validationFailures; } - private static void addAggNames(AggregationBuilder aggregationBuilder, List names) { + 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(PipelineAggregationBuilder pipelineAggregationBuilder, List names) { + private static void addAggNames(PipelineAggregationBuilder pipelineAggregationBuilder, Collection names) { names.add(pipelineAggregationBuilder.getName()); } } From 84c7cfa02408642bfc239557bc9d77e1bc86ce22 Mon Sep 17 00:00:00 2001 From: Benjamin Trent <4357155+benwtrent@users.noreply.github.com> Date: Wed, 8 May 2019 15:52:47 -0500 Subject: [PATCH 3/4] addressing PR comments --- .../transforms/pivot/PivotConfig.java | 24 +++-- .../transforms/pivot/PivotConfigTests.java | 102 +++++++++--------- 2 files changed, 65 insertions(+), 61 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java index 8f4168ad3922a..e55e7d0b877b1 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java @@ -149,6 +149,18 @@ public boolean isValid() { return groups.isValid() && aggregationConfig.isValid(); } + public List aggFieldValidation() { + if ((aggregationConfig.isValid() && groups.isValid()) == false) { + 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)); + usedNames.addAll(groups.getGroups().keySet()); + return aggFieldValidation(usedNames); + } + public static PivotConfig fromXContent(final XContentParser parser, boolean lenient) throws IOException { return lenient ? LENIENT_PARSER.apply(parser, null) : STRICT_PARSER.apply(parser, null); } @@ -165,20 +177,14 @@ public static PivotConfig fromXContent(final XContentParser parser, boolean leni * aggName2: foo.bar * * This should fail as aggName1 will cause foo.bar to be an object, causing a conflict with the use of foo.bar in aggName2. - * + * @param usedNames The aggregation and group_by names * @return List of validation failure messages */ - public List aggFieldValidation() { - if ((aggregationConfig.isValid() && groups.isValid()) == false) { + static List aggFieldValidation(List usedNames) { + if (usedNames == null || usedNames.isEmpty()) { return Collections.emptyList(); } - List validationFailures = new ArrayList<>(); - List usedNames = new ArrayList<>(); - // TODO this will need changed once we allow multi-bucket aggs + field merging - aggregationConfig.getAggregatorFactories().forEach(agg -> addAggNames(agg, usedNames)); - aggregationConfig.getPipelineAggregatorFactories().forEach(agg -> addAggNames(agg, usedNames)); - usedNames.addAll(groups.getGroups().keySet()); Set leafNames = new HashSet<>(usedNames.size()); for(String name : usedNames) { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java index 1de135bff6f91..b174fb3ba0bfa 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.core.dataframe.transforms.pivot; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.XContentParser; @@ -13,9 +14,12 @@ import org.elasticsearch.xpack.core.dataframe.transforms.AbstractSerializingDataFrameTestCase; import java.io.IOException; +import java.util.Arrays; import java.util.List; -import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.empty; public class PivotConfigTests extends AbstractSerializingDataFrameTestCase { @@ -139,72 +143,66 @@ public void testDoubleAggs() { expectThrows(IllegalArgumentException.class, () -> createPivotConfigFromString(pivot, false)); } - public void testAggNameValidations() throws IOException { + public void testValidAggNames() throws IOException { String pivotAggs = "{" + " \"group_by\": {" - + " \"user\": {" + + " \"user.id.field\": {" + " \"terms\": {" + " \"field\": \"id\"" + "} } }," + " \"aggs\": {" - + " \"user.avg\": {" + + " \"avg.field.value\": {" + " \"avg\": {" + " \"field\": \"points\"" + "} } } }"; PivotConfig pivotConfig = createPivotConfigFromString(pivotAggs, true); assertTrue(pivotConfig.isValid()); List fieldValidation = pivotConfig.aggFieldValidation(); - assertFalse(fieldValidation.isEmpty()); - assertThat(fieldValidation.get(0), equalTo("field [user] cannot be both an object and a field")); + assertTrue(fieldValidation.isEmpty()); + } - pivotAggs = "{" - + " \"group_by\": {" - + " \"user\": {" - + " \"terms\": {" - + " \"field\": \"id\"" - + "} } }," - + " \"aggs\": {" - + " \"user\": {" - + " \"avg\": {" - + " \"field\": \"points\"" - + "} } } }"; - pivotConfig = createPivotConfigFromString(pivotAggs, true); - assertTrue(pivotConfig.isValid()); - fieldValidation = pivotConfig.aggFieldValidation(); - assertFalse(fieldValidation.isEmpty()); - assertThat(fieldValidation.get(0), equalTo("duplicate field [user] detected")); + public void testAggNameValidationsWithoutIssues() { + String prefix = randomAlphaOfLength(10) + "1"; + String prefix2 = randomAlphaOfLength(10) + "2"; + String nestedField1 = randomAlphaOfLength(10) + "3"; + String nestedField2 = randomAlphaOfLength(10) + "4"; + + assertThat(PivotConfig.aggFieldValidation(Arrays.asList(prefix + nestedField1 + nestedField2, + prefix + nestedField1, + prefix, + prefix2)), is(empty())); + + assertThat(PivotConfig.aggFieldValidation( + Arrays.asList( + dotJoin(prefix, nestedField1, nestedField2), + dotJoin(nestedField1, nestedField2), + nestedField2, + prefix2)), is(empty())); + } - pivotAggs = "{" - + " \"group_by\": {" - + " \"user\": {" - + " \"terms\": {" - + " \"field\": \"id\"" - + "} } }," - + " \"aggs\": {" - + " \"avg\": {" - + " \"avg\": {" - + " \"field\": \"points\"" - + "} } } }"; - pivotConfig = createPivotConfigFromString(pivotAggs, true); - assertTrue(pivotConfig.isValid()); - fieldValidation = pivotConfig.aggFieldValidation(); - assertTrue(fieldValidation.isEmpty()); + public void testAggNameValidationsWithDuplicatesAndNestingIssues() { + String prefix = randomAlphaOfLength(10) + "1"; + String prefix2 = randomAlphaOfLength(10) + "2"; + String nestedField1 = randomAlphaOfLength(10) + "3"; + String nestedField2 = randomAlphaOfLength(10) + "4"; + + List failures = PivotConfig.aggFieldValidation( + Arrays.asList( + dotJoin(prefix, nestedField1, nestedField2), + dotJoin(prefix, nestedField2), + dotJoin(prefix, nestedField1), + dotJoin(prefix2, nestedField1), + dotJoin(prefix2, nestedField1), + prefix2)); + + assertThat(failures, + containsInAnyOrder("duplicate field [" + dotJoin(prefix2, nestedField1) + "] detected", + "field [" + prefix2 + "] cannot be both an object and a field", + "field [" + dotJoin(prefix, nestedField1) + "] cannot be both an object and a field")); + } - pivotAggs = "{" - + " \"group_by\": {" - + " \"user.id.field\": {" - + " \"terms\": {" - + " \"field\": \"id\"" - + "} } }," - + " \"aggs\": {" - + " \"avg.field.value\": {" - + " \"avg\": {" - + " \"field\": \"points\"" - + "} } } }"; - pivotConfig = createPivotConfigFromString(pivotAggs, true); - assertTrue(pivotConfig.isValid()); - fieldValidation = pivotConfig.aggFieldValidation(); - assertTrue(fieldValidation.isEmpty()); + private String dotJoin(String... fields) { + return Strings.arrayToDelimitedString(fields, "."); } private PivotConfig createPivotConfigFromString(String json, boolean lenient) throws IOException { From 5ddcd805756a742668227c67ec14544708c7e4e5 Mon Sep 17 00:00:00 2001 From: Benjamin Trent <4357155+benwtrent@users.noreply.github.com> Date: Thu, 9 May 2019 07:58:15 -0500 Subject: [PATCH 4/4] optmizing duplication check --- .../core/dataframe/transforms/pivot/PivotConfig.java | 12 +++--------- .../dataframe/transforms/pivot/PivotConfigTests.java | 2 +- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java index e55e7d0b877b1..79a0a7fc1bfa8 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java @@ -23,11 +23,9 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Map.Entry; import java.util.Objects; -import java.util.Set; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg; @@ -186,18 +184,14 @@ static List aggFieldValidation(List usedNames) { } List validationFailures = new ArrayList<>(); - Set leafNames = new HashSet<>(usedNames.size()); - for(String name : usedNames) { - if (leafNames.contains(name)) { - validationFailures.add("duplicate field [" + name + "] detected"); - } - leafNames.add(name); - } usedNames.sort(String::compareTo); for (int i = 0; i < usedNames.size() - 1; i++) { if (usedNames.get(i+1).startsWith(usedNames.get(i) + ".")) { validationFailures.add("field [" + usedNames.get(i) + "] cannot be both an object and a field"); } + if (usedNames.get(i+1).equals(usedNames.get(i))) { + validationFailures.add("duplicate field [" + usedNames.get(i) + "] detected"); + } } return validationFailures; } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java index b174fb3ba0bfa..342e007f21284 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfigTests.java @@ -201,7 +201,7 @@ public void testAggNameValidationsWithDuplicatesAndNestingIssues() { "field [" + dotJoin(prefix, nestedField1) + "] cannot be both an object and a field")); } - private String dotJoin(String... fields) { + private static String dotJoin(String... fields) { return Strings.arrayToDelimitedString(fields, "."); }