support negative epoch_millis timestamps #80208

rkophs · 2021-11-02T17:04:52Z

note: Several issues have been opened & closed for this regression. Assuming this PR is accepted, I'm not sure whether it's best practice to create yet another issue, or re-open an existing issue to link in this PR as the contribution guidelines ask for. Will update the PR accordingly once I get feedback 🙂 .

Relevant issues:

Failed to parse negative number(i. g. -1) with format [epoch_millis] #79135
Negative seconds of epoch are not mapped correctly to dates for values <=-1000, and big negative values throw an unexpected exception. #72123
Add back support for negative epoch values for time fields in Elasticsearch #40983

Context

ES 7.0 introduced a regression by no longer supporting negative timestamps once the epoch parsing code was migrated to use DateTimeFormatters instead of DateFormatter implementations. This regression appears to have been introduced
when the DateTimeFormatter replaced DateFormatter for parsing/formatting and the issue at hand appears to live exclusively in the parsing/formatting phase. Following the discussion in 40983, it appears the issue at hand is added complexity in parsing/formatting epoch's in order to accommodate Java 8's date-time API representation of epoch timestamps as a tuple of: a signed epoch-seconds & a non-negative unsigned fractional time (sub second precision).

Indeed, Instant uses a epochSeconds + non-negative nanosOfSecond fractional part.

The alternative is to use an ISO 8601 string to represent negative epochs. That is not something we can do without several thousand lines of client code setup to handle the conversion at ingestion, queries & responses and is one of the single largest blockers to a Elasticsearch major version upgrade on our backend.

Changes

Enhance the formatting & parsing logic to support negative epoch timestamps:

Parsing: in the presence of a negative fractional timestamp, subtract the epochSeconds portion by 1second and increase the negative fraction by 1second (i.e. compute the absolute value of its complementary fraction) so that the fraction is always positive (.e.g. "-4700.900ms" -> -4s + -700.900ms -> -5s + 299.100ms -> -5s + 299100000ns)
Formatting: reverse operation to parsing. In the presence of a negative epochSeconds & non-zero fraction, Concat the results of: adding 1second to the epochSeconds, recompute complementary fraction of the fractional part.

Many of the relevant issues allude to an edge-case that makes this difficult to handle. If this PR does not handle that edge-case, please shed light on what that edge-case is 🙇 .

ES 7.0 introduced a regression by no longer supporting negative timestamps once the epoch parsing code was migrated to use DateTimeFormatters instead of DateFormatter implementations This change adds back the support for negative epoch timestamps

cla-checker-service · 2021-11-02T17:04:56Z

💚 CLA has been signed

rkophs · 2021-11-02T17:06:29Z

^ The contributor agreement was signed an hour before the PR was made.

adagios · 2021-11-09T11:39:43Z

I would very much like that something like this is merged. The decision to not support one of the most common date formats is in my opinion very ill advised.

The added interface code that ES now needs when receiving things from other systems, because it doesn't support one of the simplest standards is difficult to accept as a good decision.

Imagine if Java removed the parsing of timestamps...

elasticmachine · 2021-12-01T00:08:50Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

rjernst · 2022-01-26T15:02:35Z

It has been many years so the details are a bit hazy for me. However, IIRC, the problem was not in the representation (which I believe I tried the same offsetting you do to make nanos of second always positive) but instead in Java’s format parsing for negative fractional seconds. I think it was something like -0.12345, the negative is lost by the time the nanos are handled. Can you add a test for this case?

br3no · 2022-02-02T13:42:56Z

@rkophs there's an issue to address the same problem in OpenSearch here: opensearch-project/OpenSearch#1991. Would you like to submit your pull request to OpenSearch? Otherwise, would you mind if I port your fix to OpenSearch?

rjernst · 2022-02-09T16:23:05Z

@elasticmachine update branch

rjernst · 2022-02-09T16:23:24Z

@elasticmachine ok to test

Java DateTimeFormatter isn't able to parse "-0". Therefore, the MILLIS portion of the parser fails for epochs such as "-0.12345". We can overcome this by introducing an optional TemporalField that represents whether the timestamp is signed. It is only created during parsing in the presence of a leading '-' char. Formatting will thus only support it if we're doing with a INSTANT_SECONDS temporalAccessor that happens to be negative.

…thub.com/rkophs/elasticsearch into support_negative_epoch_millis_timestamps

rkophs · 2022-02-20T02:04:02Z

@rjernst -- thanks for the insight. I added a test case for -0.12345. Indeed, you are correct. The DateTimeFormatter doesn't know how to parse "-0" for the millis portion of the timestamp.

I've added a bit of a "hack" to get that case to work by introducing a new TemporalField whose optional presence denotes that the timestamp is negative. This information is then able to be used by the MILLIS TemporalField to calculate the appropriate seconds/nanos parts during parsing. On the flipside, for formatting, the new TemporalField is only marked as supported for a TemporalAccessor that has a negative INSTANT_SECONDS value.

See my latest commit for details.

rjernst · 2022-02-21T15:30:10Z

@elasticmachine update branch

rjernst · 2022-02-23T21:14:42Z

@rkophs Could you please sign the CLA and I will review this more thoroughly?

rkophs · 2022-02-28T18:26:10Z

@rjernst -- I've signed that CLA several times (using every one of my several different emails registered in Github). Does Elastic's CLA automation require the email to match what's tied to the commits, etc?

I'm happy to email Elastic a copy of my signed agreement(s).

edit Looks like the CLA check just passed. So I think we're good?

rjernst

@rkophs This is a very clever approach, using a dummy field as a placeholder for negative values.

I left a couple minor comments, but the one thing I think must be addressed is that adding this support just for epoch_millis is going to make support for epoch timestamps inconsistent. Your approach should work for the epoch_second parser as well, which is in the same EpochTime class. Would you please adjust its implementation similarly?

rjernst · 2022-03-01T21:56:21Z

server/src/main/java/org/elasticsearch/common/time/EpochTime.java

            fieldValues.put(ChronoField.INSTANT_SECONDS, seconds);
            fieldValues.put(ChronoField.NANO_OF_SECOND, nanos);
            // if there is already a milli of second, we need to overwrite it
            if (fieldValues.containsKey(ChronoField.MILLI_OF_SECOND)) {
                fieldValues.put(ChronoField.MILLI_OF_SECOND, nanos / 1_000_000);
            }
+            if (fieldValues.containsKey(ChronoField.MICRO_OF_SECOND)) {


Why is this needed?

It is not needed. I added it defensively since the same logic is happening above with MILLI_OF_SECOND, but I'm happy to remove it since it's not directly related to the change at hand.

rjernst · 2022-03-01T21:59:18Z

server/src/main/java/org/elasticsearch/common/time/EpochTime.java

+                long nanos = temporal.getLong(ChronoField.NANO_OF_SECOND);
+                if (nanos % 1_000_000 != 0) {
+                    // Fractional negative timestamp.
+                    // Add 1 ms towards positive infinity because the fraction leads


This adjustment is tricky, and I had to stare at it for a while to understand. I think the comment would be more helpful if it mentions rounding. Essentially there is some extra precision that we are going to throw away by converting this to millis, and this adjustment by 1 ensures we are always rounding towards the epoch. Is that right? If so, can you improve the comment a bit?

Your understanding is correct. If we get into this branch, it means two things: 1) we're dealing with a negative epoch, 2) we're dealing with a fractional millisecond. Thus, the calculated millis invokes a precision loss that needs to be rounded towards the epoch for formatting (i.e. to a smaller magnitude for formatting, since Java date-time stores it as a negative at a higher magnitude by 1)

I'll update the comment to reflect.

rjernst · 2022-03-23T13:36:03Z

@elasticmachine update branch

rjernst · 2022-03-23T13:43:09Z

Thanks for the updates @rkophs. This looks good, I will wait for CI to complete and then merge.

mark-vieira · 2022-03-23T19:39:37Z

@elasticsearchmachine generate changelog

mark-vieira · 2022-03-23T19:46:35Z

@elasticsearchmachine generate changelog

rjernst · 2022-03-24T00:32:52Z

Thanks for your work here @rkophs! This is a clever approach.

Negative epoch timestamps are supported in 8.2.0 by pr elastic#80208

Negative epoch timestamps are supported in 8.2.0 by pr #80208

Negative epoch timestamps are supported in 8.2.0 by pr elastic#80208

Negative epoch timestamps are supported in 8.2.0 by pr #80208 Co-authored-by: QY <[email protected]>

Negative epoch timestamps are supported in 8.2.0 by pr elastic#80208 (cherry picked from commit 7b17e1b)

Negative epoch timestamps are supported in 8.2.0 by pr #80208 (cherry picked from commit 7b17e1b) Co-authored-by: QY <[email protected]>

support negative epoch_timestamps

6fce9cd

ES 7.0 introduced a regression by no longer supporting negative timestamps once the epoch parsing code was migrated to use DateTimeFormatters instead of DateFormatter implementations This change adds back the support for negative epoch timestamps

elasticsearchmachine added v8.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Nov 2, 2021

rkophs mentioned this pull request Nov 9, 2021

Failed to parse negative number(i. g. -1) with format [epoch_millis] #79135

Closed

rkophs changed the title ~~support negative epoch_timestamps~~ support negative epoch_millis timestamps Nov 9, 2021

jtibshirani added the :Core/Infra/Core Core issues without another label label Dec 1, 2021

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Dec 1, 2021

grcevski added the team-discuss label Dec 2, 2021

intrafindBreno mentioned this pull request Jan 27, 2022

Reintroduce support for negative epoch_millis in sub-second granularity opensearch-project/OpenSearch#1991

Closed

mark-vieira added v8.2.0 and removed v8.1.0 labels Feb 2, 2022

grcevski added the >bug label Feb 3, 2022

Merge branch 'master' into support_negative_epoch_millis_timestamps

a30f83a

rjernst self-assigned this Feb 9, 2022

rjernst self-requested a review February 9, 2022 16:24

Ryan Kophs added 2 commits February 19, 2022 20:50

Merge branch 'support_negative_epoch_millis_timestamps' of https://gi…

c849b93

…thub.com/rkophs/elasticsearch into support_negative_epoch_millis_timestamps

checkstyle - replace "!" with explicit "== false"

f225fcf

rkophs force-pushed the support_negative_epoch_millis_timestamps branch from dd2edc3 to f225fcf Compare February 20, 2022 03:56

avoid using Math.abs

96aeb31

Merge branch 'master' into support_negative_epoch_millis_timestamps

926bc49

rjernst requested changes Mar 1, 2022

View reviewed changes

rdnm removed the team-discuss label Mar 2, 2022

Ryan Kophs added 2 commits March 21, 2022 14:38

comment on negative epoch rounding; remove unnecessary code

7919af4

support negative epochs for epoch_seconds

bcf2293

rkophs requested a review from rjernst March 21, 2022 22:02

Merge branch 'master' into support_negative_epoch_millis_timestamps

dda79f3

rjernst approved these changes Mar 23, 2022

View reviewed changes

rjernst added 2 commits March 23, 2022 16:17

add changelog

9cbae7f

fix changelog format

66d1ec6

rjernst merged commit c0799ec into elastic:master Mar 24, 2022

amooncake added a commit to amooncake/elasticsearch that referenced this pull request Dec 16, 2022

[DOCS] Remove outdated note in Date field type

f5997c5

Negative epoch timestamps are supported in 8.2.0 by pr elastic#80208

amooncake mentioned this pull request Dec 16, 2022

[DOCS] Remove outdated note in Date field type #92408

Merged

abdonpijpelink pushed a commit that referenced this pull request Dec 20, 2022

[DOCS] Remove outdated note in Date field type (#92408)

7b17e1b

Negative epoch timestamps are supported in 8.2.0 by pr #80208

abdonpijpelink pushed a commit to abdonpijpelink/elasticsearch that referenced this pull request Dec 20, 2022

[DOCS] Remove outdated note in Date field type (elastic#92408)

304392d

Negative epoch timestamps are supported in 8.2.0 by pr elastic#80208

elasticsearchmachine pushed a commit that referenced this pull request Dec 20, 2022

[DOCS] Remove outdated note in Date field type (#92408) (#92468)

2a16a89

Negative epoch timestamps are supported in 8.2.0 by pr #80208 Co-authored-by: QY <[email protected]>

abdonpijpelink pushed a commit to abdonpijpelink/elasticsearch that referenced this pull request Dec 20, 2022

[DOCS] Remove outdated note in Date field type (elastic#92408)

5ed4d40

Negative epoch timestamps are supported in 8.2.0 by pr elastic#80208 (cherry picked from commit 7b17e1b)

abdonpijpelink added a commit that referenced this pull request Dec 21, 2022

[DOCS] Remove outdated note in Date field type (#92408) (#92470)

26acff4

Negative epoch timestamps are supported in 8.2.0 by pr #80208 (cherry picked from commit 7b17e1b) Co-authored-by: QY <[email protected]>

cbuescher mentioned this pull request Sep 19, 2023

Inserting epoch date throws illegal_argument_exception #99486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support negative epoch_millis timestamps #80208

support negative epoch_millis timestamps #80208

rkophs commented Nov 2, 2021

cla-checker-service bot commented Nov 2, 2021 •

edited

Loading

rkophs commented Nov 2, 2021

adagios commented Nov 9, 2021

elasticmachine commented Dec 1, 2021

rjernst commented Jan 26, 2022 •

edited

Loading

br3no commented Feb 2, 2022

rjernst commented Feb 9, 2022

rjernst commented Feb 9, 2022

rkophs commented Feb 20, 2022 •

edited

Loading

rjernst commented Feb 21, 2022

rjernst commented Feb 23, 2022

rkophs commented Feb 28, 2022 •

edited

Loading

rjernst left a comment

rjernst Mar 1, 2022

rkophs Mar 21, 2022 •

edited

Loading

rjernst Mar 1, 2022

rkophs Mar 21, 2022 •

edited

Loading

rjernst commented Mar 23, 2022

rjernst commented Mar 23, 2022

mark-vieira commented Mar 23, 2022

mark-vieira commented Mar 23, 2022

rjernst commented Mar 24, 2022

support negative epoch_millis timestamps #80208

support negative epoch_millis timestamps #80208

Conversation

rkophs commented Nov 2, 2021

Context

Changes

cla-checker-service bot commented Nov 2, 2021 • edited Loading

rkophs commented Nov 2, 2021

adagios commented Nov 9, 2021

elasticmachine commented Dec 1, 2021

rjernst commented Jan 26, 2022 • edited Loading

br3no commented Feb 2, 2022

rjernst commented Feb 9, 2022

rjernst commented Feb 9, 2022

rkophs commented Feb 20, 2022 • edited Loading

rjernst commented Feb 21, 2022

rjernst commented Feb 23, 2022

rkophs commented Feb 28, 2022 • edited Loading

rjernst left a comment

Choose a reason for hiding this comment

rjernst Mar 1, 2022

Choose a reason for hiding this comment

rkophs Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

rjernst Mar 1, 2022

Choose a reason for hiding this comment

rkophs Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

rjernst commented Mar 23, 2022

rjernst commented Mar 23, 2022

mark-vieira commented Mar 23, 2022

mark-vieira commented Mar 23, 2022

rjernst commented Mar 24, 2022

cla-checker-service bot commented Nov 2, 2021 •

edited

Loading

rjernst commented Jan 26, 2022 •

edited

Loading

rkophs commented Feb 20, 2022 •

edited

Loading

rkophs commented Feb 28, 2022 •

edited

Loading

rkophs Mar 21, 2022 •

edited

Loading

rkophs Mar 21, 2022 •

edited

Loading