-
Notifications
You must be signed in to change notification settings - Fork 24.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] adding pivot.max_search_page_size option for setting paging size #41920
Changes from 1 commit
e9944c4
dc2df4a
e4a4950
472f2c3
d06a5b0
7745205
7f00700
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
|
||
package org.elasticsearch.xpack.core.dataframe.transforms.pivot; | ||
|
||
import org.elasticsearch.common.Nullable; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.common.io.stream.Writeable; | ||
|
@@ -29,6 +30,7 @@ public class PivotConfig implements Writeable, ToXContentObject { | |
private static final String NAME = "data_frame_transform_pivot"; | ||
private final GroupConfig groups; | ||
private final AggregationConfig aggregationConfig; | ||
private final Integer size; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On having I think the decision on this depends on whether we imagine that a transform will always use a composite aggregation. If yes, then |
||
|
||
private static final ConstructingObjectParser<PivotConfig, Void> STRICT_PARSER = createParser(false); | ||
private static final ConstructingObjectParser<PivotConfig, Void> LENIENT_PARSER = createParser(true); | ||
|
@@ -55,33 +57,39 @@ private static ConstructingObjectParser<PivotConfig, Void> createParser(boolean | |
throw new IllegalArgumentException("Required [aggregations]"); | ||
} | ||
|
||
return new PivotConfig(groups, aggregationConfig); | ||
return new PivotConfig(groups, aggregationConfig, (Integer)args[3]); | ||
}); | ||
|
||
parser.declareObject(constructorArg(), | ||
(p, c) -> (GroupConfig.fromXContent(p, lenient)), DataFrameField.GROUP_BY); | ||
|
||
parser.declareObject(optionalConstructorArg(), (p, c) -> AggregationConfig.fromXContent(p, lenient), DataFrameField.AGGREGATIONS); | ||
parser.declareObject(optionalConstructorArg(), (p, c) -> AggregationConfig.fromXContent(p, lenient), DataFrameField.AGGS); | ||
parser.declareInt(optionalConstructorArg(), DataFrameField.SIZE); | ||
|
||
return parser; | ||
} | ||
|
||
public PivotConfig(final GroupConfig groups, final AggregationConfig aggregationConfig) { | ||
public PivotConfig(final GroupConfig groups, final AggregationConfig aggregationConfig, Integer size) { | ||
this.groups = ExceptionsHelper.requireNonNull(groups, DataFrameField.GROUP_BY.getPreferredName()); | ||
this.aggregationConfig = ExceptionsHelper.requireNonNull(aggregationConfig, DataFrameField.AGGREGATIONS.getPreferredName()); | ||
this.size = size; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could require a non-null value for this. Then from the REST action we set it to the default value if the user did not specify it. This way the default is not hidden and the user becomes aware of the parameter. We tend to follow this approach unless the default value is dynamically calculated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the current approach as I would not promote this setting like query. My opinion: should be an expert setting. |
||
} | ||
|
||
public PivotConfig(StreamInput in) throws IOException { | ||
this.groups = new GroupConfig(in); | ||
this.aggregationConfig = new AggregationConfig(in); | ||
this.size = in.readOptionalInt(); | ||
} | ||
|
||
@Override | ||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
builder.startObject(); | ||
builder.field(DataFrameField.GROUP_BY.getPreferredName(), groups); | ||
builder.field(DataFrameField.AGGREGATIONS.getPreferredName(), aggregationConfig); | ||
if (size != null) { | ||
builder.field(DataFrameField.SIZE.getPreferredName(), size); | ||
} | ||
builder.endObject(); | ||
return builder; | ||
} | ||
|
@@ -107,6 +115,7 @@ public void toCompositeAggXContent(XContentBuilder builder, Params params) throw | |
public void writeTo(StreamOutput out) throws IOException { | ||
groups.writeTo(out); | ||
aggregationConfig.writeTo(out); | ||
out.writeOptionalInt(size); | ||
} | ||
|
||
public AggregationConfig getAggregationConfig() { | ||
|
@@ -117,6 +126,11 @@ public GroupConfig getGroupConfig() { | |
return groups; | ||
} | ||
|
||
@Nullable | ||
public Integer getSize() { | ||
return size; | ||
} | ||
|
||
@Override | ||
public boolean equals(Object other) { | ||
if (this == other) { | ||
|
@@ -129,12 +143,14 @@ public boolean equals(Object other) { | |
|
||
final PivotConfig that = (PivotConfig) other; | ||
|
||
return Objects.equals(this.groups, that.groups) && Objects.equals(this.aggregationConfig, that.aggregationConfig); | ||
return Objects.equals(this.groups, that.groups) | ||
&& Objects.equals(this.aggregationConfig, that.aggregationConfig) | ||
&& Objects.equals(this.size, that.size); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(groups, aggregationConfig); | ||
return Objects.hash(groups, aggregationConfig, size); | ||
} | ||
|
||
public boolean isValid() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know
size
is what the search expects and in the context of a search it is a size. However, in datafeeds and now data frame transforms, this is better described as thescroll_size
orpage_size
. I'm not feeling too strongly about this but it's worth considering.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, rollup uses
page_size
, I findsize
to unspecific