From da5bfda5f314b95c3cce289c0ddb72208307f43a Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Fri, 15 Jun 2018 16:29:09 +0100 Subject: [PATCH] [ML] Hold ML filter items in sorted set (#31338) Filter items should be unique. They should also be sorted to make them easier to read plus save sorting in the autodetect process. --- .../xpack/core/ml/job/config/MlFilter.java | 21 +++++++++++-------- .../core/ml/job/config/MlFilterTests.java | 19 ++++++++++++----- .../rest-api-spec/test/ml/filter_crud.yml | 2 +- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java index 991f421265e..b11dfd47651 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/MlFilter.java @@ -17,11 +17,12 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.xpack.core.ml.MlMetaIndex; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Objects; +import java.util.SortedSet; +import java.util.TreeSet; public class MlFilter implements ToXContentObject, Writeable { @@ -53,9 +54,9 @@ public class MlFilter implements ToXContentObject, Writeable { private final String id; private final String description; - private final List items; + private final SortedSet items; - public MlFilter(String id, String description, List items) { + public MlFilter(String id, String description, SortedSet items) { this.id = Objects.requireNonNull(id, ID.getPreferredName() + " must not be null"); this.description = description; this.items = Objects.requireNonNull(items, ITEMS.getPreferredName() + " must not be null"); @@ -68,7 +69,8 @@ public class MlFilter implements ToXContentObject, Writeable { } else { description = null; } - items = Arrays.asList(in.readStringArray()); + items = new TreeSet<>(); + items.addAll(Arrays.asList(in.readStringArray())); } @Override @@ -103,8 +105,8 @@ public class MlFilter implements ToXContentObject, Writeable { return description; } - public List getItems() { - return new ArrayList<>(items); + public SortedSet getItems() { + return Collections.unmodifiableSortedSet(items); } @Override @@ -142,7 +144,7 @@ public class MlFilter implements ToXContentObject, Writeable { private String id; private String description; - private List items = Collections.emptyList(); + private SortedSet items = new TreeSet<>(); private Builder() {} @@ -162,12 +164,13 @@ public class MlFilter implements ToXContentObject, Writeable { } public Builder setItems(List items) { - this.items = items; + this.items = new TreeSet<>(); + this.items.addAll(items); return this; } public Builder setItems(String... items) { - this.items = Arrays.asList(items); + setItems(Arrays.asList(items)); return this; } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java index 78d87b82839..9ac6683f004 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/MlFilterTests.java @@ -11,10 +11,9 @@ import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.AbstractSerializingTestCase; import java.io.IOException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; +import java.util.TreeSet; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -40,7 +39,7 @@ public class MlFilterTests extends AbstractSerializingTestCase { } int size = randomInt(10); - List items = new ArrayList<>(size); + TreeSet items = new TreeSet<>(); for (int i = 0; i < size; i++) { items.add(randomAlphaOfLengthBetween(1, 20)); } @@ -58,7 +57,7 @@ public class MlFilterTests extends AbstractSerializingTestCase { } public void testNullId() { - NullPointerException ex = expectThrows(NullPointerException.class, () -> new MlFilter(null, "", Collections.emptyList())); + NullPointerException ex = expectThrows(NullPointerException.class, () -> new MlFilter(null, "", new TreeSet<>())); assertEquals(MlFilter.ID.getPreferredName() + " must not be null", ex.getMessage()); } @@ -88,4 +87,14 @@ public class MlFilterTests extends AbstractSerializingTestCase { MlFilter.LENIENT_PARSER.apply(parser, null); } } + + public void testItemsAreSorted() { + MlFilter filter = MlFilter.builder("foo").setItems("c", "b", "a").build(); + assertThat(filter.getItems(), contains("a", "b", "c")); + } + + public void testGetItemsReturnsUnmodifiable() { + MlFilter filter = MlFilter.builder("foo").setItems("c", "b", "a").build(); + expectThrows(UnsupportedOperationException.class, () -> filter.getItems().add("x")); + } } diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml index a1f7eee0dcc..2b7b86673e0 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml @@ -22,7 +22,7 @@ setup: filter_id: filter-foo body: > { - "items": ["abc", "xyz"] + "items": ["xyz", "abc"] } - do: