[ML-DataFrame] add support for fixed_interval, calendar_interval, remove interval #42427
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Pinging @elastic/ml-core |
.../main/java/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSource.java
Show resolved
Hide resolved
.../main/java/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
...n/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
...n/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
...n/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
| data_frame.preview_data_frame_transform: | ||
| body: > | ||
| { | ||
| "source": { "index": "airline-data" }, | ||
| "pivot": { | ||
| "group_by": { | ||
| "airline": {"terms": {"field": "airline"}}, | ||
| "by-hour": {"date_histogram": {"interval": "1h", "field": "time", "format": "yyyy-MM-DD HH"}}}, | ||
| "by-hour": {"date_histogram": {"fixed_interval": "1h", "field": "time", "format": "yyyy-MM-DD HH"}}}, |
There was a problem hiding this comment.
Is it worth to add a test for "calendar_interval" as well?
There was a problem hiding this comment.
we should add better test coverage for sure, however I think it is ok to do them as follow ups.
.../main/java/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSource.java
Show resolved
Hide resolved
|
run elasticsearch-ci/1 |
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
| builder.field(NAME); |
There was a problem hiding this comment.
One more nit: I've just realized it can be compressed to one line using:
return builder.field(NAME, interval);
This call uses the following method, seems this is exactly what is needed here:
public XContentBuilder field(String name, ToXContent value) throws IOException;
There was a problem hiding this comment.
The way it's done at the moment the params are preserved. It doesn't matter now but in the future it could make a difference.
.../main/java/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSource.java
Show resolved
Hide resolved
.../main/java/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSource.java
Show resolved
Hide resolved
...n/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
droberts195
reviewed
May 24, 2019
...n/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
...n/java/org/elasticsearch/xpack/core/dataframe/transforms/pivot/DateHistogramGroupSource.java
Outdated
Show resolved
Hide resolved
added 10 commits
May 24, 2019 14:12
przemekwitek
approved these changes
May 24, 2019
| private Interval readInterval(StreamInput in) throws IOException { | ||
| byte id = in.readByte(); | ||
| switch (id) { | ||
| case 0: |
There was a problem hiding this comment.
[optional]
To avoid magic numbers leaking from the Interval implementations I would:
- Introduce "static final byte INTERVAL_TYPE_ID" fields in both FixedInterval and CalendarInterval classes.
- Return them from "getIntervalTypeId()" method
- Use them here in "switch" clause instead of byte literals
hendrikmuhs
pushed a commit
that referenced
this pull request
May 24, 2019
…ove interval (#42427) * add support for fixed_interval, calendar_interval, remove interval * adapt HLRC * checkstyle * add a hlrc to server test * adapt yml test * improve naming and doc * improve interface and add test code for hlrc to server * address review comments * repair merge conflict * fix date patterns * address review comments * remove assert for warning * improve exception message * use constants
gurkankaymak
pushed a commit
to gurkankaymak/elasticsearch
that referenced
this pull request
May 27, 2019
…ove interval (elastic#42427) * add support for fixed_interval, calendar_interval, remove interval * adapt HLRC * checkstyle * add a hlrc to server test * adapt yml test * improve naming and doc * improve interface and add test code for hlrc to server * address review comments * repair merge conflict * fix date patterns * address review comments * remove assert for warning * improve exception message * use constants
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
date_histogram interval got deprecated in #33727
data frames should use the new syntax. There is no need to support the old syntax as this feature will be released with 7.2.
fixes #42297