EQL: Allow null tiebreakers inside ordinals/sequences (#65033)

Align Ordinal comparator to consider nulls last (higher) in tiebreakers.
Add unit tests to Ordinal comparisons and criterion extraction.

Fix #64706

(cherry picked from commit 93dc883abd6b8855ff1618a574412b7f773b8ff5)
(cherry picked from commit 936e5f1a2cc29c1d5662cb8aa90c629af563a987)

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/Criterion.java

@@ -25,7 +25,7 @@ public class Criterion<Q extends QueryRequest> {
 
     private final boolean descending;
 
-    Criterion(int stage,
+    public Criterion(int stage,
               Q queryRequest,
               List<HitExtractor> keys,
               HitExtractor timestamp,
@@ -83,7 +83,7 @@ public class Criterion<Q extends QueryRequest> {
 
         if (tiebreaker != null) {
             Object tb = tiebreaker.extract(hit);
-            if (tb instanceof Comparable == false) {
+            if (tb != null && tb instanceof Comparable == false) {
                 throw new EqlIllegalArgumentException("Expected tiebreaker to be Comparable but got {}", tb);
             }
             tbreaker = (Comparable<Object>) tb;

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/Ordinal.java

@@ -58,17 +58,15 @@ public class Ordinal implements Comparable<Ordinal> {
         }
         if (timestamp == o.timestamp) {
             if (tiebreaker != null) {
-                if (o.tiebreaker != null) {
-                    return tiebreaker.compareTo(o.tiebreaker);
-                }
-                // nulls are first - lower than any other value
-                // other tiebreaker is null this one isn't, fall through 1
+                // if the other tiebreaker is null, it is higher (nulls are last)
+                return o.tiebreaker != null ? tiebreaker.compareTo(o.tiebreaker) : -1;
             }
-            // null tiebreaker
+            // this tiebreaker is null
             else {
-                if (o.tiebreaker != null) {
-                    return -1;
-                } else {
+                // nulls are last so unless both are null (equal)
+                // this ordinal is greater (after) then the other tiebreaker
+                // so fall through to 1
+                if (o.tiebreaker == null) {
                     return 0;
                 }
             }

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/search/CriterionOrdinalExtractionTests.java

@@ -0,0 +1,105 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+package org.elasticsearch.xpack.eql.execution.search;
+
+import org.elasticsearch.common.document.DocumentField;
+import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.text.Text;
+import org.elasticsearch.search.SearchHit;
+import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.xpack.eql.EqlIllegalArgumentException;
+import org.elasticsearch.xpack.eql.execution.assembler.BoxedQueryRequest;
+import org.elasticsearch.xpack.eql.execution.assembler.Criterion;
+import org.elasticsearch.xpack.eql.execution.search.extractor.FieldHitExtractor;
+import org.elasticsearch.xpack.ql.execution.search.extractor.HitExtractor;
+import org.elasticsearch.xpack.ql.type.DataTypes;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import static java.util.Collections.emptyList;
+import static java.util.Collections.emptyMap;
+import static java.util.Collections.singletonList;
+
+public class CriterionOrdinalExtractionTests extends ESTestCase {
+    private String tsField = "timestamp";
+    private String tbField = "tiebreaker";
+
+    private HitExtractor tsExtractor = new FieldHitExtractor(tsField, tsField, DataTypes.LONG, null, true, null, false);
+    private HitExtractor tbExtractor = new FieldHitExtractor(tbField, tbField, DataTypes.LONG, null, true, null, false);
+
+    public void testTimeOnly() throws Exception {
+        long time = randomLong();
+        Ordinal ordinal = ordinal(searchHit(time, null), false);
+        assertEquals(time, ordinal.timestamp());
+        assertNull(ordinal.tiebreaker());
+    }
+
+    public void testTimeAndTiebreaker() throws Exception {
+        long time = randomLong();
+        long tb = randomLong();
+        Ordinal ordinal = ordinal(searchHit(time, tb), true);
+        assertEquals(time, ordinal.timestamp());
+        assertEquals(tb, ordinal.tiebreaker());
+    }
+
+    public void testTimeAndTiebreakerNull() throws Exception {
+        long time = randomLong();
+        Ordinal ordinal = ordinal(searchHit(time, null), true);
+        assertEquals(time, ordinal.timestamp());
+        assertNull(ordinal.tiebreaker());
+    }
+
+    public void testTimeNotComparable() throws Exception {
+        HitExtractor badExtractor = new FieldHitExtractor(tsField, tsField, DataTypes.BINARY, null, true, null, false);
+        SearchHit hit = searchHit(randomAlphaOfLength(10), null);
+        Criterion<BoxedQueryRequest> criterion = new Criterion<BoxedQueryRequest>(0, null, emptyList(), badExtractor, null, false);
+        EqlIllegalArgumentException exception = expectThrows(EqlIllegalArgumentException.class, () -> criterion.ordinal(hit));
+        assertTrue(exception.getMessage().startsWith("Expected timestamp"));
+    }
+
+    public void testTiebreakerNotComparable() throws Exception {
+        final Object o = randomDateTimeZone();
+        HitExtractor badExtractor = new HitExtractor() {
+            @Override
+            public Object extract(SearchHit hit) {
+                return o;
+            }
+
+            @Override
+            public String hitName() {
+                return null;
+            }
+
+            @Override
+            public String getWriteableName() {
+                return null;
+            }
+
+            @Override
+            public void writeTo(StreamOutput out) throws IOException {
+            }
+        };
+        SearchHit hit = searchHit(randomLong(), o);
+        Criterion<BoxedQueryRequest> criterion = new Criterion<BoxedQueryRequest>(0, null, emptyList(), tsExtractor, badExtractor, false);
+        EqlIllegalArgumentException exception = expectThrows(EqlIllegalArgumentException.class, () -> criterion.ordinal(hit));
+        assertTrue(exception.getMessage().startsWith("Expected tiebreaker"));
+    }
+
+    private SearchHit searchHit(Object timeValue, Object tiebreakerValue) {
+        Map<String, DocumentField> fields = new HashMap<>();
+        fields.put(tsField, new DocumentField(tsField, singletonList(timeValue)));
+        fields.put(tbField, new DocumentField(tsField, singletonList(tiebreakerValue)));
+
+        return new SearchHit(randomInt(), randomAlphaOfLength(10), new Text("_doc"), fields, emptyMap());
+    }
+
+    private Ordinal ordinal(SearchHit hit, boolean withTiebreaker) {
+        return new Criterion<BoxedQueryRequest>(0, null, emptyList(), tsExtractor, withTiebreaker ? tbExtractor : null, false).ordinal(hit);
+    }
+}

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/search/OrdinalTests.java

@@ -0,0 +1,107 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+package org.elasticsearch.xpack.eql.execution.search;
+
+import org.elasticsearch.test.ESTestCase;
+
+@SuppressWarnings("unchecked")
+public class OrdinalTests extends ESTestCase {
+
+    public void testCompareToDifferentTs() {
+        Ordinal one = new Ordinal(randomLong(), (Comparable) randomLong());
+        Ordinal two = new Ordinal(randomLong(), (Comparable) randomLong());
+
+        assertEquals(Long.valueOf(one.timestamp()).compareTo(two.timestamp()), one.compareTo(two));
+    }
+
+    public void testCompareToSameTsDifferentTie() {
+        Long ts = randomLong();
+        Ordinal one = new Ordinal(ts, (Comparable) randomLong());
+        Ordinal two = new Ordinal(ts, (Comparable) randomLong());
+
+        assertEquals(one.tiebreaker().compareTo(two.tiebreaker()), one.compareTo(two));
+    }
+
+    public void testCompareToSameTsOneTieNull() {
+        Long ts = randomLong();
+        Ordinal one = new Ordinal(ts, (Comparable) randomLong());
+        Ordinal two = new Ordinal(ts, null);
+
+        assertEquals(-1, one.compareTo(two));
+    }
+
+    @SuppressWarnings("rawtypes")
+    public void testCompareToSameTsSameTie() {
+        Long ts = randomLong();
+        Comparable c = randomLong();
+        Ordinal one = new Ordinal(ts, c);
+        Ordinal two = new Ordinal(ts, c);
+
+        assertEquals(0, one.compareTo(two));
+        assertEquals(0, one.compareTo(one));
+        assertEquals(0, two.compareTo(two));
+    }
+
+    public void testCompareToSameTsSameTieNull() {
+        Long ts = randomLong();
+        Ordinal one = new Ordinal(ts, null);
+        Ordinal two = new Ordinal(ts, null);
+
+        assertEquals(0, one.compareTo(two));
+        assertEquals(0, one.compareTo(one));
+        assertEquals(0, two.compareTo(two));
+    }
+
+    public void testTestBetween() {
+        Ordinal before = new Ordinal(randomLongBetween(1000, 2000), (Comparable) randomLong());
+        Ordinal between = new Ordinal(randomLongBetween(3000, 4000), (Comparable) randomLong());
+        Ordinal after = new Ordinal(randomLongBetween(5000, 6000), (Comparable) randomLong());
+
+        assertTrue(before.between(before, after));
+        assertTrue(after.between(before, after));
+        assertTrue(between.between(before, after));
+
+        assertFalse(new Ordinal(randomLongBetween(0, 999), null).between(before, after));
+        assertFalse(new Ordinal(randomLongBetween(7000, 8000), null).between(before, after));
+    }
+
+    public void testTestBefore() {
+        Ordinal before = new Ordinal(randomLongBetween(1000, 2000), (Comparable) randomLong());
+        Ordinal after = new Ordinal(randomLongBetween(5000, 6000), (Comparable) randomLong());
+
+        assertTrue(before.before(after));
+        assertFalse(before.before(before));
+        assertFalse(after.before(before));
+    }
+
+    public void testBeforeOrAt() {
+        Ordinal before = new Ordinal(randomLongBetween(1000, 2000), (Comparable) randomLong());
+        Ordinal after = new Ordinal(randomLongBetween(5000, 6000), (Comparable) randomLong());
+
+        assertTrue(before.beforeOrAt(after));
+        assertTrue(before.beforeOrAt(before));
+        assertFalse(after.beforeOrAt(before));
+    }
+
+    public void testTestAfter() {
+        Ordinal before = new Ordinal(randomLongBetween(1000, 2000), (Comparable) randomLong());
+        Ordinal after = new Ordinal(randomLongBetween(5000, 6000), (Comparable) randomLong());
+
+        assertTrue(after.after(before));
+        assertFalse(after.after(after));
+        assertFalse(before.after(after));
+    }
+
+    public void testAfterOrAt() {
+        Ordinal before = new Ordinal(randomLongBetween(1000, 2000), (Comparable) randomLong());
+        Ordinal after = new Ordinal(randomLongBetween(5000, 6000), (Comparable) randomLong());
+
+        assertTrue(after.afterOrAt(before));
+        assertTrue(after.afterOrAt(after));
+        assertFalse(before.afterOrAt(after));
+    }
+}