Skip to content

Commit 08e4f4b

Browse files
tlrxtlrx
authored
[Rollup] Remove builders from HistoGroupConfig (#32533)
Related to #29827
1 parent 4cdbb42 commit 08e4f4b

File tree

8 files changed

+97
-143
lines changed

8 files changed

+97
-143
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/GroupConfig.java

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -44,34 +44,34 @@ public class GroupConfig implements Writeable, ToXContentObject {
4444
private static final ParseField TERMS = new ParseField("terms");
4545

4646
private final DateHistoGroupConfig dateHisto;
47-
private final HistoGroupConfig histo;
47+
private final HistogramGroupConfig histo;
4848
private final TermsGroupConfig terms;
4949

5050
public static final ObjectParser<GroupConfig.Builder, Void> PARSER = new ObjectParser<>(NAME, GroupConfig.Builder::new);
5151

5252
static {
5353
PARSER.declareObject(GroupConfig.Builder::setDateHisto, (p,c) -> DateHistoGroupConfig.PARSER.apply(p,c).build(), DATE_HISTO);
54-
PARSER.declareObject(GroupConfig.Builder::setHisto, (p,c) -> HistoGroupConfig.PARSER.apply(p,c).build(), HISTO);
54+
PARSER.declareObject(GroupConfig.Builder::setHisto, (p,c) -> HistogramGroupConfig.fromXContent(p), HISTO);
5555
PARSER.declareObject(GroupConfig.Builder::setTerms, (p,c) -> TermsGroupConfig.fromXContent(p), TERMS);
5656
}
5757

58-
private GroupConfig(DateHistoGroupConfig dateHisto, @Nullable HistoGroupConfig histo, @Nullable TermsGroupConfig terms) {
58+
private GroupConfig(DateHistoGroupConfig dateHisto, @Nullable HistogramGroupConfig histo, @Nullable TermsGroupConfig terms) {
5959
this.dateHisto = Objects.requireNonNull(dateHisto, "A date_histogram group is mandatory");
6060
this.histo = histo;
6161
this.terms = terms;
6262
}
6363

6464
GroupConfig(StreamInput in) throws IOException {
6565
dateHisto = new DateHistoGroupConfig(in);
66-
histo = in.readOptionalWriteable(HistoGroupConfig::new);
66+
histo = in.readOptionalWriteable(HistogramGroupConfig::new);
6767
terms = in.readOptionalWriteable(TermsGroupConfig::new);
6868
}
6969

7070
public DateHistoGroupConfig getDateHisto() {
7171
return dateHisto;
7272
}
7373

74-
public HistoGroupConfig getHisto() {
74+
public HistogramGroupConfig getHisto() {
7575
return histo;
7676
}
7777

@@ -83,7 +83,7 @@ public Set<String> getAllFields() {
8383
Set<String> fields = new HashSet<>();
8484
fields.add(dateHisto.getField());
8585
if (histo != null) {
86-
fields.addAll(histo.getAllFields());
86+
fields.addAll(asList(histo.getFields()));
8787
}
8888
if (terms != null) {
8989
fields.addAll(asList(terms.getFields()));
@@ -109,9 +109,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
109109
dateHisto.toXContent(builder, params);
110110
builder.endObject();
111111
if (histo != null) {
112-
builder.startObject(HISTO.getPreferredName());
113-
histo.toXContent(builder, params);
114-
builder.endObject();
112+
builder.field(HISTO.getPreferredName(), histo);
115113
}
116114
if (terms != null) {
117115
builder.field(TERMS.getPreferredName(), terms);
@@ -156,7 +154,7 @@ public String toString() {
156154

157155
public static class Builder {
158156
private DateHistoGroupConfig dateHisto;
159-
private HistoGroupConfig histo;
157+
private HistogramGroupConfig histo;
160158
private TermsGroupConfig terms;
161159

162160
public DateHistoGroupConfig getDateHisto() {
@@ -168,11 +166,11 @@ public GroupConfig.Builder setDateHisto(DateHistoGroupConfig dateHisto) {
168166
return this;
169167
}
170168

171-
public HistoGroupConfig getHisto() {
169+
public HistogramGroupConfig getHisto() {
172170
return histo;
173171
}
174172

175-
public GroupConfig.Builder setHisto(HistoGroupConfig histo) {
173+
public GroupConfig.Builder setHisto(HistogramGroupConfig histo) {
176174
this.histo = histo;
177175
return this;
178176
}

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/HistoGroupConfig.java renamed to ‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/HistogramGroupConfig.java

Lines changed: 39 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,10 @@
1212
import org.elasticsearch.common.io.stream.StreamInput;
1313
import org.elasticsearch.common.io.stream.StreamOutput;
1414
import org.elasticsearch.common.io.stream.Writeable;
15-
import org.elasticsearch.common.xcontent.ObjectParser;
16-
import org.elasticsearch.common.xcontent.ToXContentFragment;
15+
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
16+
import org.elasticsearch.common.xcontent.ToXContentObject;
1717
import org.elasticsearch.common.xcontent.XContentBuilder;
18+
import org.elasticsearch.common.xcontent.XContentParser;
1819
import org.elasticsearch.search.aggregations.bucket.composite.CompositeValuesSourceBuilder;
1920
import org.elasticsearch.search.aggregations.bucket.composite.HistogramValuesSourceBuilder;
2021
import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
@@ -27,9 +28,10 @@
2728
import java.util.List;
2829
import java.util.Map;
2930
import java.util.Objects;
30-
import java.util.Set;
3131
import java.util.stream.Collectors;
3232

33+
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
34+
3335
/**
3436
* The configuration object for the histograms in the rollup config
3537
*
@@ -42,28 +44,36 @@
4244
* ]
4345
* }
4446
*/
45-
public class HistoGroupConfig implements Writeable, ToXContentFragment {
46-
private static final String NAME = "histo_group_config";
47-
public static final ObjectParser<HistoGroupConfig.Builder, Void> PARSER
48-
= new ObjectParser<>(NAME, HistoGroupConfig.Builder::new);
47+
public class HistogramGroupConfig implements Writeable, ToXContentObject {
4948

50-
private static final ParseField INTERVAL = new ParseField("interval");
51-
private static final ParseField FIELDS = new ParseField("fields");
49+
public static final String NAME = "histogram";
50+
private static final String INTERVAL = "interval";
51+
private static final String FIELDS = "fields";
52+
private static final ConstructingObjectParser<HistogramGroupConfig, Void> PARSER;
53+
static {
54+
PARSER = new ConstructingObjectParser<>(NAME, args -> {
55+
@SuppressWarnings("unchecked") List<String> fields = (List<String>) args[1];
56+
return new HistogramGroupConfig((long) args[0], fields != null ? fields.toArray(new String[fields.size()]) : null);
57+
});
58+
PARSER.declareLong(constructorArg(), new ParseField(INTERVAL));
59+
PARSER.declareStringArray(constructorArg(), new ParseField(FIELDS));
60+
}
5261

5362
private final long interval;
5463
private final String[] fields;
5564

56-
static {
57-
PARSER.declareLong(HistoGroupConfig.Builder::setInterval, INTERVAL);
58-
PARSER.declareStringArray(HistoGroupConfig.Builder::setFields, FIELDS);
59-
}
60-
61-
private HistoGroupConfig(long interval, String[] fields) {
65+
public HistogramGroupConfig(final long interval, final String... fields) {
66+
if (interval <= 0) {
67+
throw new IllegalArgumentException("Interval must be a positive long");
68+
}
69+
if (fields == null || fields.length == 0) {
70+
throw new IllegalArgumentException("Fields must have at least one value");
71+
}
6272
this.interval = interval;
6373
this.fields = fields;
6474
}
6575

66-
HistoGroupConfig(StreamInput in) throws IOException {
76+
HistogramGroupConfig(final StreamInput in) throws IOException {
6777
interval = in.readVLong();
6878
fields = in.readStringArray();
6979
}
@@ -101,18 +111,14 @@ public List<CompositeValuesSourceBuilder<?>> toBuilders() {
101111
public Map<String, Object> toAggCap() {
102112
Map<String, Object> map = new HashMap<>(2);
103113
map.put("agg", HistogramAggregationBuilder.NAME);
104-
map.put(INTERVAL.getPreferredName(), interval);
114+
map.put(INTERVAL, interval);
105115
return map;
106116
}
107117

108118
public Map<String, Object> getMetadata() {
109119
return Collections.singletonMap(RollupField.formatMetaField(RollupField.INTERVAL), interval);
110120
}
111121

112-
public Set<String> getAllFields() {
113-
return Arrays.stream(fields).collect(Collectors.toSet());
114-
}
115-
116122
public void validateMappings(Map<String, Map<String, FieldCapabilities>> fieldCapsResponse,
117123
ActionRequestValidationException validationException) {
118124

@@ -138,9 +144,13 @@ public void validateMappings(Map<String, Map<String, FieldCapabilities>> fieldCa
138144
}
139145

140146
@Override
141-
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
142-
builder.field(INTERVAL.getPreferredName(), interval);
143-
builder.field(FIELDS.getPreferredName(), fields);
147+
public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException {
148+
builder.startObject();
149+
{
150+
builder.field(INTERVAL, interval);
151+
builder.field(FIELDS, fields);
152+
}
153+
builder.endObject();
144154
return builder;
145155
}
146156

@@ -151,19 +161,15 @@ public void writeTo(StreamOutput out) throws IOException {
151161
}
152162

153163
@Override
154-
public boolean equals(Object other) {
164+
public boolean equals(final Object other) {
155165
if (this == other) {
156166
return true;
157167
}
158-
159168
if (other == null || getClass() != other.getClass()) {
160169
return false;
161170
}
162-
163-
HistoGroupConfig that = (HistoGroupConfig) other;
164-
165-
return Objects.equals(this.interval, that.interval)
166-
&& Arrays.equals(this.fields, that.fields);
171+
final HistogramGroupConfig that = (HistogramGroupConfig) other;
172+
return Objects.equals(interval, that.interval) && Arrays.equals(fields, that.fields);
167173
}
168174

169175
@Override
@@ -176,36 +182,7 @@ public String toString() {
176182
return Strings.toString(this, true, true);
177183
}
178184

179-
public static class Builder {
180-
private long interval = 0;
181-
private List<String> fields;
182-
183-
public long getInterval() {
184-
return interval;
185-
}
186-
187-
public HistoGroupConfig.Builder setInterval(long interval) {
188-
this.interval = interval;
189-
return this;
190-
}
191-
192-
public List<String> getFields() {
193-
return fields;
194-
}
195-
196-
public HistoGroupConfig.Builder setFields(List<String> fields) {
197-
this.fields = fields;
198-
return this;
199-
}
200-
201-
public HistoGroupConfig build() {
202-
if (interval <= 0) {
203-
throw new IllegalArgumentException("Parameter [" + INTERVAL.getPreferredName() + "] must be a positive long.");
204-
}
205-
if (fields == null || fields.isEmpty()) {
206-
throw new IllegalArgumentException("Parameter [" + FIELDS + "] must have at least one value.");
207-
}
208-
return new HistoGroupConfig(interval, fields.toArray(new String[0]));
209-
}
185+
public static HistogramGroupConfig fromXContent(final XContentParser parser) throws IOException {
186+
return PARSER.parse(parser, null);
210187
}
211188
}

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/ConfigTestHelpers.java

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,13 @@
55
*/
66
package org.elasticsearch.xpack.core.rollup;
77

8+
import com.carrotsearch.randomizedtesting.generators.RandomNumbers;
89
import org.elasticsearch.common.unit.TimeValue;
910
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;
1011
import org.elasticsearch.test.ESTestCase;
1112
import org.elasticsearch.xpack.core.rollup.job.DateHistoGroupConfig;
1213
import org.elasticsearch.xpack.core.rollup.job.GroupConfig;
13-
import org.elasticsearch.xpack.core.rollup.job.HistoGroupConfig;
14+
import org.elasticsearch.xpack.core.rollup.job.HistogramGroupConfig;
1415
import org.elasticsearch.xpack.core.rollup.job.MetricConfig;
1516
import org.elasticsearch.xpack.core.rollup.job.RollupJobConfig;
1617
import org.elasticsearch.xpack.core.rollup.job.TermsGroupConfig;
@@ -50,7 +51,7 @@ public static GroupConfig.Builder getGroupConfig() {
5051
GroupConfig.Builder groupBuilder = new GroupConfig.Builder();
5152
groupBuilder.setDateHisto(getDateHisto().build());
5253
if (ESTestCase.randomBoolean()) {
53-
groupBuilder.setHisto(getHisto().build());
54+
groupBuilder.setHisto(randomHistogramGroupConfig(ESTestCase.random()));
5455
}
5556
if (ESTestCase.randomBoolean()) {
5657
groupBuilder.setTerms(randomTermsGroupConfig(ESTestCase.random()));
@@ -102,13 +103,6 @@ public static DateHistoGroupConfig.Builder getDateHisto() {
102103
return dateHistoBuilder;
103104
}
104105

105-
public static HistoGroupConfig.Builder getHisto() {
106-
HistoGroupConfig.Builder histoBuilder = new HistoGroupConfig.Builder();
107-
histoBuilder.setInterval(ESTestCase.randomIntBetween(1,10000));
108-
histoBuilder.setFields(getFields());
109-
return histoBuilder;
110-
}
111-
112106
public static List<String> getFields() {
113107
return IntStream.range(0, ESTestCase.randomIntBetween(1, 10))
114108
.mapToObj(n -> ESTestCase.randomAlphaOfLengthBetween(5, 10))
@@ -125,6 +119,10 @@ public static String getCronString() {
125119
" " + (ESTestCase.randomBoolean() ? "*" : String.valueOf(ESTestCase.randomIntBetween(1970, 2199))); //year
126120
}
127121

122+
public static HistogramGroupConfig randomHistogramGroupConfig(final Random random) {
123+
return new HistogramGroupConfig(randomInterval(random), randomFields(random));
124+
}
125+
128126
public static TermsGroupConfig randomTermsGroupConfig(final Random random) {
129127
return new TermsGroupConfig(randomFields(random));
130128
}
@@ -141,4 +139,8 @@ private static String[] randomFields(final Random random) {
141139
private static String randomField(final Random random) {
142140
return randomAsciiAlphanumOfLengthBetween(random, 5, 10);
143141
}
142+
143+
private static long randomInterval(final Random random) {
144+
return RandomNumbers.randomLongBetween(random, 1L, Long.MAX_VALUE);
145+
}
144146
}

0 commit comments

Comments
 (0)