[ML] adding pivot.max_search_page_size option for setting paging size #41920

benwtrent · 2019-05-07T21:32:17Z

This adds a size parameter to the pivot configuration in a data frame transform.

I put the parameter inside the pivot configuration and not in the root object, as it relates directly to how we are pivoting the data. Since we are unsure what the next transform looks like, I thought it best to restricted it instead of making it a transform field.

If it is null we still opt to use the internal default of 500.

elasticmachine · 2019-05-07T21:32:19Z

Pinging @elastic/ml-core

dimitris-athanasiou

Looks good. A few points to discuss.

dimitris-athanasiou · 2019-05-08T07:12:47Z

.../core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java

        this.groups = ExceptionsHelper.requireNonNull(groups, DataFrameField.GROUP_BY.getPreferredName());
        this.aggregationConfig = ExceptionsHelper.requireNonNull(aggregationConfig, DataFrameField.AGGREGATIONS.getPreferredName());
+        this.size = size;


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.

I like the current approach as I would not promote this setting like query. My opinion: should be an expert setting.

dimitris-athanasiou · 2019-05-08T07:15:07Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/dataframe/DataFrameField.java

@@ -27,6 +27,7 @@
    public static final ParseField SOURCE = new ParseField("source");
    public static final ParseField DESTINATION = new ParseField("dest");
    public static final ParseField FORCE = new ParseField("force");
+    public static final ParseField SIZE = new ParseField("size");


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 the scroll_size or page_size. I'm not feeling too strongly about this but it's worth considering.

+1, rollup uses page_size, I find size to unspecific

dimitris-athanasiou · 2019-05-08T07:22:34Z

.../core/src/main/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/PivotConfig.java

@@ -29,6 +30,7 @@
    private static final String NAME = "data_frame_transform_pivot";
    private final GroupConfig groups;
    private final AggregationConfig aggregationConfig;
+    private final Integer size;


On having size in pivot instead of the transform object itself:

I think the decision on this depends on whether we imagine that a transform will always use a composite aggregation. If yes, then size should be on the top level.

hendrikmuhs · 2019-05-08T07:58:24Z

Looks good, we should decide on the naming. For me this is an expert setting, so I would not promote it.

API level I find it confusing from the user perspective: the usage of composite aggs is an implementation detail and a parameter "size" isn't descriptive enough.

It would be better to give it a more descriptive, explicit name: max_search_page_size (I like the very descriptive names in CCR: https://www.elastic.co/guide/en/elasticsearch/reference/current/ccr-put-follow.html). Long names make usage harder, but this is an expert setting.

About placement: If it's just 1 setting it's ok to put it into pivot, however I already see that we will have more settings in future, therefore I think it will get confusing.

My proposal, a top-level parameters section together with very descriptive names:

{
  "source": {...},
  "dest": {...},
  "pivot": {...},
  "parameters": {
      "max_search_page_size": 1000,
      ...
   }
}

The parameters might not apply to the function used in the transform, but I think that's simpler than having a parameters objects in every function. If a parameter does not make sense in the context of the function I would just ignore that, as said this is expert level.

…-max-page-size-option

benwtrent · 2019-05-09T15:10:57Z

run elasticsearch-ci/1

benwtrent · 2019-05-09T20:50:45Z

run elasticsearch-ci/1

hendrikmuhs · 2019-05-10T06:11:46Z

run elasticsearch-ci/1

hendrikmuhs

LGTM

dimitris-athanasiou

LGTM

…elastic#41920) * [ML] adding pivot.size option for setting paging size * Changing field name to address PR comments * fixing ctor usage * adjust hlrc for field name change

…#41920) (#42079) * [ML] adding pivot.size option for setting paging size * Changing field name to address PR comments * fixing ctor usage * adjust hlrc for field name change

* elastic/master: (84 commits) [ML] adds geo_centroid aggregation support to data frames (elastic#42088) Add documentation for calendar/fixed intervals (elastic#41919) Remove global checkpoint assertion in peer recovery (elastic#41987) Don't create tempdir for cli scripts (elastic#41913) Fix debian-8 update (elastic#42056) Cleanup plugin bin directories (elastic#41907) Prevent order being lost for _nodes API filters (elastic#42045) Change IndexAnalyzers default analyzer access (elastic#42011) Remove reference to fs.data.spins in docs Mute failing AsyncTwoPhaseIndexerTests Remove close method in PageCacheRecycler/Recycler (elastic#41917) [ML] adding pivot.max_search_page_size option for setting paging size (elastic#41920) Docs: Tweak list formatting Simplify handling of keyword field normalizers (elastic#42002) [ML] properly nesting objects in document source (elastic#41901) Remove extra `ms` from log message (elastic#42068) Increase the sample space for random inner hits name generator (elastic#42057) Recognise direct buffers in heap size docs (elastic#42070) shouldRollGeneration should execute under read lock (elastic#41696) Wait for active shard after close in mixed cluster (elastic#42029) ...

…elastic#41920) * [ML] adding pivot.size option for setting paging size * Changing field name to address PR comments * fixing ctor usage * adjust hlrc for field name change

benwtrent added >non-issue v8.0.0 v7.2.0 :ml/Transform Transform labels May 7, 2019

benwtrent force-pushed the feature/ml-df-add-max-page-size-option branch from 1def6de to 8cbb8e7 Compare May 7, 2019 21:41

jasontedor force-pushed the master branch from 1b80e0d to 8af01df Compare May 7, 2019 23:35

[ML] adding pivot.size option for setting paging size

e9944c4

benwtrent force-pushed the feature/ml-df-add-max-page-size-option branch from 8cbb8e7 to e9944c4 Compare May 8, 2019 00:04

dimitris-athanasiou reviewed May 8, 2019

View reviewed changes

benwtrent added 3 commits May 8, 2019 15:54

Merge branch 'master' into feature/ml-df-add-max-page-size-option

dc2df4a

Merge remote-tracking branch 'upstream/master' into feature/ml-df-add…

e4a4950

…-max-page-size-option

Changing field name to address PR comments

472f2c3

benwtrent changed the title ~~[ML] adding pivot.size option for setting paging size~~ [ML] adding pivot.max_search_page_size option for setting paging size May 8, 2019

fixing ctor usage

d06a5b0

Merge branch 'master' into feature/ml-df-add-max-page-size-option

7745205

benwtrent requested review from hendrikmuhs and dimitris-athanasiou and removed request for hendrikmuhs May 9, 2019 16:13

adjust hlrc for field name change

7f00700

hendrikmuhs approved these changes May 10, 2019

View reviewed changes

dimitris-athanasiou approved these changes May 10, 2019

View reviewed changes

benwtrent merged commit da4899f into elastic:master May 10, 2019

benwtrent deleted the feature/ml-df-add-max-page-size-option branch May 10, 2019 14:31

benwtrent mentioned this pull request May 10, 2019

[ML] adding pivot.max_search_page_size option for setting paging size #42079

Merged

szabosteve mentioned this pull request Jul 9, 2019

[DOCS] Adds max_page_search_size to data frame transform pivot properties #44126

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] adding pivot.max_search_page_size option for setting paging size #41920

[ML] adding pivot.max_search_page_size option for setting paging size #41920

benwtrent commented May 7, 2019

elasticmachine commented May 7, 2019

dimitris-athanasiou left a comment

dimitris-athanasiou May 8, 2019

hendrikmuhs May 8, 2019

dimitris-athanasiou May 8, 2019 •

edited

Loading

hendrikmuhs May 8, 2019

dimitris-athanasiou May 8, 2019

hendrikmuhs commented May 8, 2019 •

edited

Loading

benwtrent commented May 9, 2019

benwtrent commented May 9, 2019

hendrikmuhs commented May 10, 2019

hendrikmuhs left a comment

dimitris-athanasiou left a comment

[ML] adding pivot.max_search_page_size option for setting paging size #41920

[ML] adding pivot.max_search_page_size option for setting paging size #41920

Conversation

benwtrent commented May 7, 2019

elasticmachine commented May 7, 2019

dimitris-athanasiou left a comment

Choose a reason for hiding this comment

dimitris-athanasiou May 8, 2019

Choose a reason for hiding this comment

hendrikmuhs May 8, 2019

Choose a reason for hiding this comment

dimitris-athanasiou May 8, 2019 • edited Loading

Choose a reason for hiding this comment

hendrikmuhs May 8, 2019

Choose a reason for hiding this comment

dimitris-athanasiou May 8, 2019

Choose a reason for hiding this comment

hendrikmuhs commented May 8, 2019 • edited Loading

benwtrent commented May 9, 2019

benwtrent commented May 9, 2019

hendrikmuhs commented May 10, 2019

hendrikmuhs left a comment

Choose a reason for hiding this comment

dimitris-athanasiou left a comment

Choose a reason for hiding this comment

dimitris-athanasiou May 8, 2019 •

edited

Loading

hendrikmuhs commented May 8, 2019 •

edited

Loading