EQL: Sequences will now support nano-timestamps by bpintea · Pull Request #76953 · elastic/elasticsearch

bpintea · 2021-08-25T20:19:42Z

With this change nanosecond-resolution timestamps can be used to drive
the EQL sequence query.

With this change nanosecond-resolution timestamps can be used to drive the EQL sequence query.

bpintea · 2021-08-25T20:33:48Z

+        // 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.


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.

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.

bpintea · 2021-08-25T20:33:54Z

 name = "sequencesOnDifferentEventTypes7"
 query = '''
-sequence with maxspan=500ms
+sequence with maxspan=50ms


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.

Better to keep the current test as is and add another one (for nanos).

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.

bpintea · 2021-08-25T20:33:59Z

+    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


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.

astefan

Left few comments.

astefan · 2021-08-26T12:36:53Z

+        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();


Here entry.get("timestamp") is already inside object.

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.

I meant

Suggested change

String timestamp = entry.get("timestamp").toString();

String timestamp = object.toString();

Ah, you're right, thanks (inside vs assigned threw me off).
I'll fix this with any next round of changes.

astefan · 2021-08-26T12:40:04Z

+        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);


Why this change?

For date_nanos field type needs to either provide millis or an ISO9601 format. The value of the object is however holding the nanos.

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?

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).

astefan · 2021-08-26T15:58:07Z

        }
-        long implicitTiebreaker = ((Number) implicitTbreaker).longValue();
-        return new Ordinal(timestamp, tbreaker, implicitTiebreaker);
+        return new Ordinal((Number) ts, tbreaker, (Number) implicitTbreaker);


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).

Thanks, reverted these changes.

astefan · 2021-08-26T16:02:03Z

+    private final Number timestamp;
    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


Same here: long.

- replace BigDecimal with Instant. - undo implicitTiebreaker changes.

elasticmachine · 2021-08-27T07:51:36Z

Pinging @elastic/es-ql (Team:QL)

Luegg · 2021-08-30T07:09:15Z

    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;


👍 for the explanations

Luegg · 2021-08-30T07:27:11Z

        // 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.


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.

Good point, thanks, added.

Luegg · 2021-08-30T08:08:12Z

                stageToKeys);
    }
+
+    private static long timestampDiffNanos(Object ts1, Object ts2) {


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.

This has been now refactored and mostly covered by OrdinalTests.java.

Luegg · 2021-08-30T08:09:01Z

+            } else {
+                diff = i1.minusMillis(((Number) ts2).longValue());
+            }
+            timestampDiffNanos = diff.getEpochSecond() * 1_000_000_000L + diff.getNano();


Suggested change

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.

Refactored.

Luegg · 2021-08-30T08:13:26Z

+
+    static Object randomTimestamp() {
+        if (randomBoolean()) {
+            return randomLongBetween(-3155701416721920000L, 3155688986440319900L);


where do these bounds come from? Will timestamps never be outside of these ranges?

Those came from Instant, that has some (private) ranges for seconds. But this has been now improved.

Luegg · 2021-08-30T08:16:59Z

    private static final long SHALLOW_SIZE = RamUsageEstimator.shallowSizeOfInstance(Ordinal.class);

-    private final long timestamp;
+    private final Object timestamp;


I'm probably missing quite some context, but why not always using instant for holding the ts?

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.

astefan

LGTM in general. I've left some questions.

astefan · 2021-08-30T19:59:10Z

+        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);


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?

astefan · 2021-08-30T20:41:02Z

 name = "filterOnDateNanosWithMillis"
 query = '''
-    process where @timestamp == "1970-01-02T12:31:25.510Z"
+    process where @timestamp == "1974-03-02T19:53:16.510Z"


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.

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).

astefan · 2021-08-30T20:47:33Z

+        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()));


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?

This has been refactored.

astefan · 2021-08-30T20:50:51Z

                // 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) });


Leftover from undo-ing the implicit tiebreaker changes?

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.

costin

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.

costin · 2021-08-31T07:38:45Z

- * - 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


a better name might be endgame-140-nanos to indicate it's another copy of endgame-140.

costin · 2021-08-31T07:44:07Z

 name = "sequencesOnDifferentEventTypes7"
 query = '''
-sequence with maxspan=500ms
+sequence with maxspan=50ms


Better to keep the current test as is and add another one (for nanos).

costin · 2021-08-31T08:03:26Z

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

costin

Left some comments around cosmetics - LGTM.

costin · 2021-09-01T10:07:09Z


-    // time delta in nanos from other to instance
+    // time delta in nanos between this and other instance
    public long diff(Timestamp other) {


This could be improved by NANOS.between(this, other) which behind the scenes does the math computation in this method.

Also delta, until or between sound better than diff.

Went for delta. (until suggests that other is expected to be past this, which is not and between might require a range argument)

costin · 2021-09-01T10:08:49Z

+
+    @Override
+    public int compareTo(Timestamp other) {
+        return other instanceof MillisTimestamp ? Long.compare(timestamp, ((MillisTimestamp) other).timestamp) : -other.compareTo(this);


to avoid the surprising - you could just delegate to the parent - super.compareTo(this).

Reverted to previous format.

costin · 2021-09-01T10:09:01Z

+
+    @Override
+    public long diff(Timestamp other) {
+        return other instanceof MillisTimestamp ? (timestamp - ((MillisTimestamp) other).timestamp) * NANOS_PER_MILLI : -other.diff(this);


costin · 2021-09-01T10:11:25Z

        @Override
-        public Long extract(SearchHit hit) {
-            return (long) hit.docId();
+        public Object extract(SearchHit hit) {


The return time should be Timestamp not Object.

Fixed, thanks.

- revert to using 'super' in MillisTimestamp; - rename Timestamp#timestamp() to #instant().

Delegate nanos arithmetics.

Remove no longer unused constant.

elasticsearchmachine · 2021-09-01T19:21:32Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run backport --upstream elastic/elasticsearch --pr 76953

With this change nanosecond-resolution timestamps can be used to drive the EQL sequence query. (cherry picked from commit 9570236)

…77147) With this change nanosecond-resolution timestamps can be used to drive the EQL sequence query. (cherry picked from commit 9570236)

Sequences will now support nano-timestamps

763c61e

With this change nanosecond-resolution timestamps can be used to drive the EQL sequence query.

bpintea added >enhancement v8.0.0 :Analytics/EQL EQL querying v7.16.0 labels Aug 25, 2021

bpintea requested review from astefan and costin August 25, 2021 20:19

bpintea commented Aug 25, 2021

View reviewed changes

Comment thread server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java Outdated

astefan reviewed Aug 26, 2021

View reviewed changes

bpintea added 2 commits August 27, 2021 08:23

Merge branch 'master' into feat/eql_sequence_nanos

a4a5679

Adress review comments

a6dd270

- replace BigDecimal with Instant. - undo implicitTiebreaker changes.

bpintea marked this pull request as ready for review August 27, 2021 07:51

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Aug 27, 2021

bpintea requested a review from Luegg August 27, 2021 07:54

Luegg reviewed Aug 30, 2021

View reviewed changes

astefan approved these changes Aug 30, 2021

View reviewed changes

costin requested changes Aug 31, 2021

View reviewed changes

bpintea added 4 commits August 31, 2021 18:28

Address review comments

ad90d3b

- wrap long and Instance timestamps into a Timestamp small hierarchy. - small test fixes and commenting.

Merge branch 'master' into feat/eql_sequence_nanos

2ba7386

Address review comments

b97d3ab

- cache MillisTimestamp conversion to Instant; - merge and streamline some of the Timestamp methods. - add comment on changed test

Merge branch 'master' into feat/eql_sequence_nanos

db31711

costin approved these changes Sep 1, 2021

View reviewed changes

bpintea added 3 commits September 1, 2021 12:44

Address review comments

35b9d0c

- revert to using 'super' in MillisTimestamp; - rename Timestamp#timestamp() to #instant().

Merge branch 'master' into feat/eql_sequence_nanos

ff31707

Delegate nanos arithmetics

5bd470a

Delegate nanos arithmetics.

Remove no longer unused constant

bc77ffb

Remove no longer unused constant.

bpintea added the auto-backport-and-merge label Sep 1, 2021

bpintea merged commit 9570236 into elastic:master Sep 1, 2021

bpintea deleted the feat/eql_sequence_nanos branch September 1, 2021 19:20

bpintea added the backport pending label Sep 1, 2021

bpintea removed the backport pending label Sep 2, 2021

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

		// 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.

	String timestamp = entry.get("timestamp").toString();
	String timestamp = object.toString();

	timestampDiffNanos = diff.getEpochSecond() * 1_000_000_000L + diff.getNano();
	return diff.getEpochSecond() * 1_000_000_000L + diff.getNano();

Uh oh!

Conversation

bpintea commented Aug 25, 2021

Uh oh!

bpintea Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpintea Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bpintea Aug 25, 2021 •

edited

Loading

bpintea Aug 25, 2021 •

edited

Loading