Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -170,4 +170,46 @@ public int hashCode() {
public String toString() {
return Strings.toString(this, true, true);
}

public static Builder builder() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would make sense to make DataFrameTransformConfig constructor private now that we have "builder()" as a recommended entry point to this class? The same question for other classes...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good thought, How about "package private" so that tests can still utilize it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Package-private SGTM

return new Builder();
}

public static class Builder {

private String id;
private SourceConfig source;
private DestConfig dest;
private PivotConfig pivotConfig;
private String description;

public Builder setId(String id) {
this.id = id;
return this;
}

public Builder setSource(SourceConfig source) {
this.source = source;
return this;
}

public Builder setDest(DestConfig dest) {
this.dest = dest;
return this;
}

public Builder setPivotConfig(PivotConfig pivotConfig) {
this.pivotConfig = pivotConfig;
return this;
}

public Builder setDescription(String description) {
this.description = description;
return this;
}

public DataFrameTransformConfig build() {
return new DataFrameTransformConfig(id, source, dest, pivotConfig, description);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.query.QueryBuilder;

import java.io.IOException;
import java.util.Arrays;
Expand Down Expand Up @@ -121,4 +122,32 @@ public int hashCode(){
int hash = Arrays.hashCode(index);
return 31 * hash + (queryConfig == null ? 0 : queryConfig.hashCode());
}

public static Builder builder() {
return new Builder();
}

public static class Builder {
private String[] index;
private QueryConfig queryConfig;

public Builder setIndex(String... index) {
this.index = index;
return this;
}

public Builder setQueryConfig(QueryConfig queryConfig) {
this.queryConfig = queryConfig;
return this;
}

public Builder setQuery(QueryBuilder query) {
this.queryConfig = new QueryConfig(query);
return this;
}

public SourceConfig build() {
return new SourceConfig(index, queryConfig);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -168,4 +168,55 @@ public boolean equals(Object other) {
public int hashCode() {
return Objects.hash(field, interval, dateHistogramInterval, timeZone, format);
}

public static Builder builder() {
return new Builder();
}

public static class Builder {

private String field;
private long interval = 0;
private DateHistogramInterval dateHistogramInterval;
private String format;
private ZoneId timeZone;

public Builder setField(String field) {
this.field = field;
return this;
}

public Builder setInterval(long interval) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this method accept "Duration" instead of "long"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to make this parallel the DateHistogramAggregationBuilder. Though I should add docs stating what time resolution interval is.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if we talk about usability here, it is usually error-prone if the user has to provide "long" but does not know what the unit is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@przemekwitek I added some docs and a new method that accepts a TimeValue parameter as well.

this.interval = interval;
return this;
}

public Builder setDateHistgramInterval(DateHistogramInterval interval) {
this.dateHistogramInterval = interval;
return this;
}

public Builder setFormat(String format) {
this.format = format;
return this;
}

public Builder setTimeZone(ZoneId timeZone) {
this.timeZone = timeZone;
return this;
}

public DateHistogramGroupSource build() {
DateHistogramGroupSource groupSource = new DateHistogramGroupSource(field);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing Builder is a great opportunity to make the data class itself (here, DateHistogramGroupSource) immutable. Do you plan on making it so or you find such an approach too restrictive?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We totally should do this. I will see if I can make them final.

if (dateHistogramInterval != null) {
groupSource.setDateHistogramInterval(dateHistogramInterval);
}
if (interval >= 1.0) {
groupSource.setInterval(interval);
}
groupSource.setFormat(format);
groupSource.setTimeZone(timeZone);
return groupSource;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -174,4 +175,25 @@ public int hashCode() {
public String toString() {
return Strings.toString(this, true, true);
}

public static Builder builder() {
return new Builder();
}

public static class Builder {
private final Map<String, SingleGroupSource> groups;

public Builder() {
this.groups = new HashMap<>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could move initialization to line 184.

}

public Builder groupBy(String name, SingleGroupSource group) {
groups.put(name, group);
return this;
}

public GroupConfig build() {
return new GroupConfig(groups);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,28 @@ public boolean equals(Object other) {
public int hashCode() {
return Objects.hash(field, interval);
}

public static Builder builder() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, I cannot leave comment in the line 55. Anyway, leaving it here: Please make the constructor package-private

return new Builder();
}

public static class Builder {

private String field;
private double interval;

public Builder setField(String field) {
this.field = field;
return this;
}

public Builder setInterval(double interval) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question, should we use "Duration" here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that does not make sense here as interval in this case is any arbitrary number. Any numeric field can be grouped via a histogram, not just a time field.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A, ok. So this was my misunderstanding (I associated "interval" with some time duration).

this.interval = interval;
return this;
}

public HistogramGroupSource build() {
return new HistogramGroupSource(field, interval);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.search.aggregations.AggregatorFactories;

import java.io.IOException;
import java.util.Objects;
Expand Down Expand Up @@ -96,4 +97,32 @@ public int hashCode() {
public boolean isValid() {
return groups.isValid() && aggregationConfig.isValid();
}

public static Builder builder() {
return new Builder();
}

public static class Builder {
private GroupConfig groups;
private AggregationConfig aggregationConfig;

public Builder setGroups(GroupConfig groups) {
this.groups = groups;
return this;
}

public Builder setAggregationConfig(AggregationConfig aggregationConfig) {
this.aggregationConfig = aggregationConfig;
return this;
}

public Builder setAggregations(AggregatorFactories.Builder aggregations) {
this.aggregationConfig = new AggregationConfig(aggregations);
return this;
}

public PivotConfig build() {
return new PivotConfig(groups, aggregationConfig);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,22 @@ public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params par
builder.endObject();
return builder;
}

public static Builder builder() {
return new Builder();
}

public static class Builder {

private String field;

public Builder setField(String field) {
this.field = field;
return this;
}

public TermsGroupSource build() {
return new TermsGroupSource(field);
}
}
}