-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
[Transform] use ISO dates in output instead of epoch millis #65584
[Transform] use ISO dates in output instead of epoch millis #65584
Conversation
…bs and old behavior fixes elastic#63787
4f2f197
to
56a0ba5
Compare
...src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java
Show resolved
Hide resolved
...st/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByIT.java
Outdated
Show resolved
Hide resolved
...st/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByIT.java
Outdated
Show resolved
Hide resolved
...in/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java
Show resolved
Hide resolved
...h-level/src/test/java/org/elasticsearch/client/transform/transforms/SettingsConfigTests.java
Outdated
Show resolved
Hide resolved
...h-level/src/test/java/org/elasticsearch/client/transform/transforms/SettingsConfigTests.java
Outdated
Show resolved
Hide resolved
@elasticmachine update branch |
@szabosteve Can you have a look at the doc changes: elasticsearch_65584.docs-preview.app.elstc.co/diff |
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.
Docs changes LGTM, thanks.
It's just a nit, so please take it or leave it: we try to order the parameters alphabetically in the API docs and in the common params file (I know, we are not always consistent...). If you don't have time for changing it, no problem, common-parms
needs to be reviewed anyway soon.
I fixed it. I blame the last minute rename of the parameter, the 1st version was |
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.
LGTM
with just two small remarks
...gin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/SettingsConfig.java
Show resolved
Hide resolved
...in/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java
Outdated
Show resolved
Hide resolved
) (#65952) Transform writes dates as epoch millis, this does not work for historic data in some cases or is unsupported. Dates should be written as such. With this PR transform starts writing dates in ISO format, but as existing transform might rely on the format it provides backwards compatibility for old jobs as well as a setting to write dates as epoch millis. fixes #63787 backport #65584
finalize the integration of #65584
Pinging @elastic/ml-core (:ml/Transform) |
Transform writes dates as epoch millis, this does not work for historic data in some cases or is unsupported. Dates should be written as such. With this PR transform starts writing dates in ISO format, but as existing transform might rely on the format it provides backwards compatibility for old jobs as well as a setting to write dates as epoch millis.
fixes #63787
Example
The following example illustrates the change, we use the following config for pivot (dataset:
kibana_sample_data_logs
):The change affects how
@timestamp
is written in_source
(note: dates in the aggs part are already written as ISO string, with the change dates inpivot.group_by
andpivot.aggregations
behave consistently):Before
After
Doc values do not change, because either way it is parsed as
date
(but parsing can't fail anymore if dates<1970
).In case you rely on the old format (because you use the produced
_source
in an application), you can get back the old style with:When updating an old transform using
_update
, this is automatically set. If you want to migrate an old transform to the new style, you can use_update
to explicitly setdates_as_epoch_millis
tofalse
ornull
(== default).Docs preview for the added parameter: https://elasticsearch_65584.docs-preview.app.elstc.co/diff
Post PR actions: