-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
EQL: Sequences will now support nano-timestamps #76953
Conversation
With this change nanosecond-resolution timestamps can be used to drive the EQL sequence query.
// Only parse as BigDecimal if needed by nanos accuracy (when getting millis with sub-millis). Long nanos will only count up to | ||
// 2262-04-11T23:47:16.854775807Z. Doubles are not accurate enough to hold six digit submillis with granularity for current dates. |
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.
To the above points, using BigDecimal here comes with the advantage of being able to use the same scale for both millis- and nanos-timestamps; i.e. documents containing millis vs nanos can be compared on millis, no nano-scaling needed.
The alternative would be using the readily available BigInteger (no server changes needed), but Ordinal comparisons would require scaling.
Another alternative might be not to support dates past the Long nano resolution (the above date).
Or, maybe introducing EQL's own class (to hold two longs) or use a DateTime class, but that would hardly improve things.
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.
Fwiw, indexing a nano-resolution date past 2262-04-11T23:47:16.854775807Z
is rejected by ES. Querying a merged date
+date_nanos
mapping with an indexed date
past the previous limit fails "to create query" with a search_phase_execution_exception
as well. The need to distinguish between date
and date_nanos
within the same query remains, though, so we need to use a different class than Long for holding the date_nanos
.
Switched this from BigDecimal
to Instant
for already existing serialisation support.
@@ -861,7 +861,7 @@ expected_event_ids = [67, 68, 69, 70, | |||
[[queries]] | |||
name = "sequencesOnDifferentEventTypes7" | |||
query = ''' | |||
sequence with maxspan=500ms | |||
sequence with maxspan=50ms |
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.
There are two sequences matching this test with nanos, at a timespan of 50.232ms (vs 5s+ with millis timestamps). Reducing the value here makes the test usable with both millis and nanos tests.
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.
Better to keep the current test as is and add another one (for nanos).
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.
I've added a comment to the test and explained further the change intention, to allow the same sequence queries be executed on both millis and nanos timestamps.
private static void timestampToUnixNanos(Map<String, Object> entry) { | ||
Object object = entry.get("timestamp"); | ||
assertThat(object, instanceOf(Long.class)); | ||
// interpret the value as nanos since the unix epoch |
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.
The chosen Windows filetime timestamps (2017+) can coincidently also be readily used as nano-resolution unix timestamps (1973+). Which warrants conveniently using the same endgame-140.data
file also the nano tests.
server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java
Outdated
Show resolved
Hide resolved
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.
Left few comments.
Object object = entry.get("timestamp"); | ||
assertThat(object, instanceOf(Long.class)); | ||
// interpret the value as nanos since the unix epoch | ||
String timestamp = entry.get("timestamp").toString(); |
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.
Here entry.get("timestamp")
is already inside object
.
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.
It is, but as a Long instance and we need a string, otherwise passing the float through the JSON to ES will eventually have ES try to parse the scientific string representation of the float as an Instant, which fails.
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.
I meant
String timestamp = entry.get("timestamp").toString(); | |
String timestamp = object.toString(); |
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.
Ah, you're right, thanks (inside vs assigned threw me off).
I'll fix this with any next round of changes.
String milliFraction = timestamp.substring(12); | ||
// strip the fractions right away if not actually present | ||
entry.put("@timestamp", milliFraction.equals("000000") ? millis : millis + "." + milliFraction); | ||
entry.put("timestamp", ((long) object)/1_000_000L); |
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.
Why this change?
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.
For date_nanos
field type needs to either provide millis or an ISO9601 format. The value of the object is however holding the nanos.
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.
I meant why do you need timestamp
to hold the same value as it did before, but without the nanos in it. Is timestamp
actually used in tests?
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.
Just like for the existing millis tests, timestamp
is populated but not used in the qa tests. Having it with a meaningful value allows however quick millis vs nanos comparison tests though (used mostly for troubleshooting, only changing the timestamp_field
).
@@ -96,8 +94,7 @@ public Ordinal ordinal(SearchHit hit) { | |||
if (implicitTbreaker instanceof Number == false) { | |||
throw new EqlIllegalArgumentException("Expected _shard_doc/implicit tiebreaker as long but got [{}]", implicitTbreaker); | |||
} | |||
long implicitTiebreaker = ((Number) implicitTbreaker).longValue(); | |||
return new Ordinal(timestamp, tbreaker, implicitTiebreaker); | |||
return new Ordinal((Number) ts, tbreaker, (Number) implicitTbreaker); |
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.
I suggest leaving this as a primitive long
since the tiebreaker is/should always be a long
and should always exist (vs the eql tiebreaker which is optional).
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.
Thanks, reverted these changes.
private final Comparable<Object> tiebreaker; | ||
private final long implicitTiebreaker; // _shard_doc tiebreaker automatically added by ES PIT | ||
private final Number implicitTiebreaker; // _shard_doc tiebreaker automatically added by ES PIT |
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.
Same here: long
.
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.
Ditto.
- replace BigDecimal with Instant. - undo implicitTiebreaker changes.
Pinging @elastic/es-ql (Team:QL) |
@@ -56,8 +59,8 @@ | |||
|
|||
private static final Map<String, String[]> replacementPatterns = Collections.unmodifiableMap(getReplacementPatterns()); | |||
|
|||
private static final long FILETIME_EPOCH_DIFF = 11644473600000L; | |||
private static final long FILETIME_ONE_MILLISECOND = 10 * 1000; |
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.
👍 for the explanations
// | ||
// Date_Nanos index | ||
// | ||
// The data for this index are identical to the endgame-140.data with only the values for @timestamp changed. | ||
// The data for this index is loaded from the same endgame-140.data sample, only having the mapping for @timestamp changed. |
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.
maybe this would also be a good place to explain the differences in the interpretation of the timestamp values? I think future maintainers would be grateful for having this (unix epoch vs windows filetime) pointed out a bit more prominently.
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.
Good point, thanks, added.
@@ -339,4 +340,27 @@ public String toString() { | |||
completed.size(), | |||
stageToKeys); | |||
} | |||
|
|||
private static long timestampDiffNanos(Object ts1, Object ts2) { |
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.
Maybe it would be good to have some dedicated unit tests for this method. Does it support diffing over the fully supported range of timestamps? Also, I can imagine the method is vulnerable to long overflow issues.
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.
This has been now refactored and mostly covered by OrdinalTests.java.
} else { | ||
diff = i1.minusMillis(((Number) ts2).longValue()); | ||
} | ||
timestampDiffNanos = diff.getEpochSecond() * 1_000_000_000L + diff.getNano(); |
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.
timestampDiffNanos = diff.getEpochSecond() * 1_000_000_000L + diff.getNano(); | |
return diff.getEpochSecond() * 1_000_000_000L + diff.getNano(); |
and the same for the other branches.
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.
Refactored.
|
||
static Object randomTimestamp() { | ||
if (randomBoolean()) { | ||
return randomLongBetween(-3155701416721920000L, 3155688986440319900L); |
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.
where do these bounds come from? Will timestamps never be outside of these ranges?
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.
Those came from Instant, that has some (private) ranges for seconds. But this has been now improved.
import java.util.Objects; | ||
|
||
public class Ordinal implements Comparable<Ordinal>, Accountable { | ||
|
||
private static final long SHALLOW_SIZE = RamUsageEstimator.shallowSizeOfInstance(Ordinal.class); | ||
|
||
private final long timestamp; | ||
private final Object timestamp; |
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.
I'm probably missing quite some context, but why not always using instant for holding the ts?
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.
Mostly for performance reasons, since timestamp handling is a core sequences operation and presumably most queries run on millis (as it had so far), i.e. longs.
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 in general. I've left some questions.
String milliFraction = timestamp.substring(12); | ||
// strip the fractions right away if not actually present | ||
entry.put("@timestamp", milliFraction.equals("000000") ? millis : millis + "." + milliFraction); | ||
entry.put("timestamp", ((long) object)/1_000_000L); |
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.
I meant why do you need timestamp
to hold the same value as it did before, but without the nanos in it. Is timestamp
actually used in tests?
@@ -4,20 +4,20 @@ | |||
[[queries]] | |||
name = "filterOnDateNanosWithMillis" | |||
query = ''' | |||
process where @timestamp == "1970-01-02T12:31:25.510Z" | |||
process where @timestamp == "1974-03-02T19:53:16.510Z" |
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.
Why this change in test data? I got from you test changes that the data itself didn't actually change, I'm curious why have you changed these tests and, at the same time, expected_event_ids
remained the same.
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.
I got from you test changes that the data itself didn't actually change
Yes, it did change: the nano tests used before a copy of endgame-140.data
that was modified to match the few tests contained in test_queries_date_nanos.toml
. With this PR eql_date_nanos.data
has been removed and tests setup to use with the same data - endgame-140.data
- with different interpretation of the timestamp value. This allows to run the same tests - test_queries.toml
on both millis and nanos. But this also requires the event nano tests - test_queries_date_nanos.toml
- changed in order to match the existing results (that are kept unchanged).
return ts1 instanceof Instant | ||
? (ts2 instanceof Instant ? ((Instant) ts1).compareTo((Instant) ts2) : | ||
((Instant) ts1).compareTo(Instant.ofEpochMilli(((Number) ts2).longValue()))) | ||
: (ts2 instanceof Instant ? Instant.ofEpochMilli(((Number) ts1).longValue()).compareTo((Instant) ts2) : | ||
Long.compare(((Number) ts1).longValue(), ((Number) ts2).longValue())); |
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.
Isn't the ternary operator being a bit overused here? Wouldn't an if/else
be more easy to follow visually?
Also, looking at the changes in this PR, having instanceof Instant
logic separate as a utility method would make more sense to show the difference between a date-with-nanos and date-with-millis?
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.
This has been refactored.
@@ -65,7 +65,7 @@ public void query(QueryRequest r, ActionListener<SearchResponse> l) { | |||
int previous = ordinal - 1; | |||
// except the first request, the rest should have the previous response's search_after _shard_doc value | |||
assertArrayEquals("Elements at stage " + ordinal + " do not match", | |||
r.searchSource().searchAfter(), new Object[] { (long) previous, implicitTiebreakerValues.get(previous) }); | |||
r.searchSource().searchAfter(), new Object[] { String.valueOf(previous), implicitTiebreakerValues.get(previous) }); |
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.
Leftover from undo-ing the implicit tiebreaker changes?
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.
This has be a string, since otherwise the value - before a Long/Instant, now a Timestamp - can't be used when processing downstream the search object.
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.
The Instant/Long handling can be better encapsulated. I'm +1 in not having a wrapper class (see my other comment) around the Long/Instant however as it stands, the code that handles the differences is spread across multiple classes making it hard to follow.
Instead I propose to consolidate it in a single class (potentially in the ordinal package) and move there all the methods that perform comparison, difference, toString() etc...
That should simplify some things since constants like 1_000_000_000 are defined once and better documented while calling classes can simply call:
timestamps.diff(ordinal.timestamp, newOrdinal.timestamp)
.
As a separate note, it's worth checking whether we can avoid some of the boxing/autoboxing happening by using Object
. Previously long
was used however due to Instant
now Object
is passed around as a signature which means anything that happens with long
creates objects - so maybe adding a wrapping class (Timestamp) with different implementations, on for long
and one for Instant
might work out after all.
* - endgame-140 - for existing data | ||
* - extra - additional data | ||
* - endgame-140 - for existing data | ||
* - eql_date_nanos - same as endgame-140, but with nano-precision timestamps |
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.
a better name might be endgame-140-nanos
to indicate it's another copy of endgame-140
.
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.
Applied.
@@ -861,7 +861,7 @@ expected_event_ids = [67, 68, 69, 70, | |||
[[queries]] | |||
name = "sequencesOnDifferentEventTypes7" | |||
query = ''' | |||
sequence with maxspan=500ms | |||
sequence with maxspan=50ms |
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.
Better to keep the current test as is and add another one (for nanos).
I'm concerned that the tests have to be changed to make this change work out - that should not be the case. |
- wrap long and Instance timestamps into a Timestamp small hierarchy. - small test fixes and commenting.
- cache MillisTimestamp conversion to Instant; - merge and streamline some of the Timestamp methods. - add comment on changed test
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.
Left some comments around cosmetics - LGTM.
@@ -42,14 +42,9 @@ public static Timestamp of(String milliseconds) { | |||
return timestamp; | |||
} | |||
|
|||
// time delta in nanos from other to instance | |||
// time delta in nanos between this and other instance | |||
public long diff(Timestamp other) { |
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.
This could be improved by NANOS.between(this, other)
which behind the scenes does the math computation in this method.
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.
Also delta, until or between sound better than 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.
Went for delta. (until
suggests that other
is expected to be past this
, which is not and between
might require a range argument)
|
||
@Override | ||
public int compareTo(Timestamp other) { | ||
return other instanceof MillisTimestamp ? Long.compare(timestamp, ((MillisTimestamp) other).timestamp) : -other.compareTo(this); |
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.
to avoid the surprising -
you could just delegate to the parent - super.compareTo(this)
.
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.
Reverted to previous format.
|
||
@Override | ||
public long diff(Timestamp other) { | ||
return other instanceof MillisTimestamp ? (timestamp - ((MillisTimestamp) other).timestamp) * NANOS_PER_MILLI : -other.diff(this); |
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.
See above.
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.
Ditto.
@@ -85,8 +86,8 @@ public String hitName() { | |||
static final TimestampExtractor INSTANCE = new TimestampExtractor(); | |||
|
|||
@Override | |||
public Long extract(SearchHit hit) { | |||
return (long) hit.docId(); | |||
public Object extract(SearchHit hit) { |
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.
The return time should be Timestamp not Object.
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.
Fixed, thanks.
- revert to using 'super' in MillisTimestamp; - rename Timestamp#timestamp() to #instant().
Delegate nanos arithmetics.
Remove no longer unused constant.
💔 Backport failed
To backport manually run |
With this change nanosecond-resolution timestamps can be used to drive the EQL sequence query. (cherry picked from commit 9570236)
With this change nanosecond-resolution timestamps can be used to drive
the EQL sequence query.
Closes #68812.