Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Dates stored in milliseconds can be passed with higher resolution losing precision #37962

Open
spinscale opened this issue Jan 29, 2019 · 9 comments · Fixed by #78921
Open

Dates stored in milliseconds can be passed with higher resolution losing precision #37962

spinscale opened this issue Jan 29, 2019 · 9 comments · Fixed by #78921
Labels
>breaking :Core/Infra/Core Core issues without another label >deprecation Team:Core/Infra Meta label for core/infra team

Comments

@spinscale
Copy link
Contributor

During the joda to java-time switch we discovered that dates can have a higher resolution, that the implementation is able to store without warning the user about this.

Having a date field allows to send a date like 2018-10-03T14:42:44.613469+0000 to Elasticsearch. This date however will not store the 469 microsecond part, but silently discard it.

We should discuss, if we want to do anything about this like emitting a warning.
@spinscale spinscale added the :Search/Mapping Index mappings, including merging and defining field types label Jan 29, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@rjernst
Copy link
Member

rjernst commented Feb 11, 2019

I think this should be a failure, not a warning. Could we detect this in the DateFieldMapper.parseCreateField method, checking nano seconds % 1000 equals 0?

@jimczi
Copy link
Contributor

jimczi commented Feb 11, 2019

We discussed this issue internally and agreed that an error should be raised. However we are concerned by the bwc of this change and more particularly by the impact that the deprecation could have. It is allowed in 6x to send a date that contains a microseconds part by default so we'd need to log a deprecation warning in this version (and for all indices created in 6x ?) for each document that contains a deprecated format. This could slow down the indexing considerably so we agreed to document the limitation first. There is no way currently to group deprecation warning issued by different documents so we need to think of a better way to deprecate this kind of behavior.

@jtibshirani jtibshirani added >deprecation >docs General docs changes labels Apr 22, 2019
@jtibshirani jtibshirani added the :Core/Infra/Core Core issues without another label label May 20, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@debadair
Copy link
Contributor

[docs issue triage]

@rjernst rjernst added Team:Core/Infra Meta label for core/infra team Team:Docs Meta label for docs team Team:Search Meta label for search team labels May 4, 2020
@rjernst rjernst added the needs:triage Requires assignment of a team area label label Dec 3, 2020
@gwbrown
Copy link
Contributor

gwbrown commented Dec 15, 2020

I'm labeling this team-discuss to determine if there is a way we need to improve the deprecation logging infrastructure to allow addressing this issue.

@gwbrown gwbrown added team-discuss and removed needs:triage Requires assignment of a team area label labels Dec 15, 2020
@jimczi jimczi removed :Search/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team labels Jan 15, 2021
@pgomulka
Copy link
Contributor

we discussed this during core-infra sync on 17th March and concluded that we would like to work on this.
I am not sure I understand the problem with deprecations @jimczi . Deprecation logs can be throttled by any arbitrary key. So we could use an index name and field name and this will prevent to emit deprecations for multiple documents given field&index.
This would be deprecated in 7.x and we would error in v8

@pgomulka pgomulka removed team-discuss >docs General docs changes labels Mar 18, 2021
@elasticmachine elasticmachine removed the Team:Docs Meta label for docs team label Mar 18, 2021
@pgomulka pgomulka mentioned this issue Oct 11, 2021
Merged
pgomulka added a commit that referenced this issue Oct 18, 2021
@pgomulka
Deprecate resolution loss on date field (#78921)
d50c9e8 
When storing nanoseconds on a date field the nanosecond part is lost as
it cannot be stored. The date_nanos field should be used instead.
This commit emits a deprecation warning to notify users about this.

closes #37962
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue Oct 18, 2021
@pgomulka
Deprecate resolution loss on date field (elastic#78921)
f3f8fc8 
When storing nanoseconds on a date field the nanosecond part is lost as
it cannot be stored. The date_nanos field should be used instead.
This commit emits a deprecation warning to notify users about this.

closes elastic#37962
@pgomulka pgomulka mentioned this issue Oct 18, 2021
Merged
pgomulka added a commit that referenced this issue Oct 18, 2021
@pgomulka
Deprecate resolution loss on date field (#78921) backport(#79355)
ff6e589 
When storing nanoseconds on a date field the nanosecond part is lost as
it cannot be stored. The date_nanos field should be used instead.
This commit emits a deprecation warning to notify users about this.

closes #37962
backports #78921
@pgomulka
Copy link
Contributor

reopening as the change was only deprecated for now

@pgomulka pgomulka reopened this Oct 20, 2021
@pgomulka
Copy link
Contributor

This deprecation ended up affecting other components in our stack (beats) and we decided that this is too late to include this in 8.0 release. Hence it was reverted.
Before considering to merge this deprecation again we need to figure out some ways to make it less invasive to users. For instance making sure dynamics mapping can appropriately choose date type, set processor with _ingest/timestamp also choosing date_nanos when setting high resolution.
Also the stack components should be notified with advance about these changes.

pgomulka added a commit that referenced this issue Oct 28, 2021
@pgomulka
Revert "Deprecate resolution loss on date field (#78921)" (#79914)
7aaa234 
This reverts commit d50c9e8.

relates #78921

reason #37962 (comment)
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue Oct 28, 2021
@pgomulka
Revert "Deprecate resolution loss on date field (elastic#78921)" (ela…
3337afe 
…stic#79914)

This reverts commit d50c9e8.

relates elastic#78921

reason elastic#37962 (comment)
@pgomulka pgomulka mentioned this issue Oct 28, 2021
Merged
pgomulka added a commit that referenced this issue Oct 28, 2021
@pgomulka
Revert "Deprecate resolution loss on date field (#78921)" (#80003)
8674678 
This reverts commit d50c9e8.

relates #78921

reason #37962 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
>breaking :Core/Infra/Core Core issues without another label >deprecation Team:Core/Infra Meta label for core/infra team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Deprecate resolution loss on date field pgomulka/elasticsearch
8 participants
@rjernst @debadair @spinscale @gwbrown @jtibshirani @pgomulka @elasticmachine @jimczi