Allow for negative unix timestamps #11482
Conversation
|
One thing to discuss is, if it makes sense to check for the length of the epoch timestamps. Right now they are limited to 10/13 chars depending on millisecond resolution, which means the max date is 2286. Maybe we should just loosen this? |
If someone is using our 200 years from now, then they have larger problems. |
|
I was more thinking about different worlds... like in games. however then one potentially should just use ISO8601 dates and not unix timestamps :) |
+1 Probably worth documenting the way it works if it isn't already but "just use ISO8601" is almost never bad advice. |
|
I actually disagree with that. ISO8601 is overly complex. RFC3339 is good enough and much simpler. Exotic use cases that don't work with it could be handled by a separate complicated ISO8601 plugin :) |
|
agreed, lets replace iso8601 with rfc3339 in my sentence above... I think it's ok to keep the validity checks. Will update the docs as part of this PR as well |
spinscale
force-pushed
the
1506-issue-11478-fix-mapping
branch
from
9453039 to
2e221f9
Compare
|
updated docs to reflect the ability to only go up/down to a certain year with a unix timestamp, also added bwc documentation |
clintongormley
changed the title
| } else if ("epoch_millis".equals(input)) { | ||
| formatter = new DateTimeFormatterBuilder().append(new EpochTimeParser(true)).toFormatter(); | ||
| formatter = new DateTimeFormatterBuilder().append(forPattern("dateOptionalTime").printer().getPrinter(), new EpochTimeParser(true)).toFormatter(); |
There was a problem hiding this comment.
it seems wrong to me to give the dateOptionTime printer to the epoch_* formats, since in that case I would not be able to print epoch dates if I needed/wanted to? I would suggest allowing these formats to print as epoch timestamps and then the user can use the order of formats in the format field to determine how they want the date to be printed.
Maybe also (in a different PR) we should separate index and display time formats into different options so you could set only e.g. epoch_second to be allowed when indexing documents but display a more human readable format to the user at query time?
There was a problem hiding this comment.
+1 on a dedicated printer
Maybe also (in a different PR) we should separate index and display time formats into different options so you could set only e.g. epoch_second to be allowed when indexing documents but display a more human readable format to the user at query time?
One thing that makes me uncomfortable about this is that it means that a format could not always parse the dates that it prints.
spinscale
force-pushed
the
1506-issue-11478-fix-mapping
branch
from
2e221f9 to
eee5550
Compare
|
added an EpochPrinter here as well... @jpountz maybe you can have a look if that helps your case, I will check if I can optimize the code a bit more instead of reading out the fields piece by piece, maybe the timestamp already exist and I can just reuse it |
| if (hasMilliSecondPrecision) { | ||
| out.write(String.valueOf(instant)); | ||
| } else { | ||
| out.append(String.valueOf(instant * 1000)); |
There was a problem hiding this comment.
shouldn't it be /1000 instead?
There was a problem hiding this comment.
fixed and test added
spinscale
force-pushed
the
1506-issue-11478-fix-mapping
branch
from
eee5550 to
ae15808
Compare
| } | ||
|
|
||
| public void testThatEpochParserIsIdempotent() { | ||
| FormatDateTimeFormatter formatter = Joda.forPattern("epoch_millis"); |
There was a problem hiding this comment.
no tests for epoch_second?
|
LGTM |
This fixes an issue to allow for negative unix timestamps. An own printer for epochs instead of just having a parser has been added. Added docs that only 10/13 length unix timestamps are supported Added docs in upgrade documentation Fixes elastic#11478
spinscale
force-pushed
the
1506-issue-11478-fix-mapping
branch
from
ae15808 to
38ddc81
Compare
clintongormley
added
the
:Search Foundations/Mapping
This fixes an issue to allow for negative unix timestamps.
It also fixes the default date field mapper to support epochs.
Fixes #11478
Fixes #11692