EQL: Deprecate lenient sequence declaration (#55032)

Deprecate alternative sequence parameter declaration (with then by)
Disallow lack of time units inside maxspan

Fix #55023
Relate #54680

(cherry picked from commit 201adafba9def1de4bf843760defb9def3394f63)

x-pack/plugin/eql/src/main/antlr/EqlBase.g4

@@ -30,7 +30,7 @@ sequenceParams
     ;
 
 sequence
-    : SEQUENCE  (by=joinKeys sequenceParams? | sequenceParams by=joinKeys?)?
+    : SEQUENCE (by=joinKeys sequenceParams? | sequenceParams disallowed=joinKeys?)?
       sequenceTerm sequenceTerm+
       (UNTIL until=sequenceTerm)?
     ;

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/EqlBaseParser.java

@@ -393,6 +393,7 @@ class EqlBaseParser extends Parser {
 
   public static class SequenceContext extends ParserRuleContext {
     public JoinKeysContext by;
+    public JoinKeysContext disallowed;
     public SequenceTermContext until;
     public TerminalNode SEQUENCE() { return getToken(EqlBaseParser.SEQUENCE, 0); }
     public List<SequenceTermContext> sequenceTerm() {
@@ -462,7 +463,7 @@ class EqlBaseParser extends Parser {
         if (_la==BY) {
           {
           setState(83);
-          ((SequenceContext)_localctx).by = joinKeys();
+          ((SequenceContext)_localctx).disallowed = joinKeys();
           }
         }
 

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/LogicalPlanBuilder.java

@@ -112,8 +112,13 @@ public abstract class LogicalPlanBuilder extends ExpressionBuilder {
 
     @Override
     public Sequence visitSequence(SequenceContext ctx) {
-        List<Expression> parentJoinKeys = visitJoinKeys(ctx.by);
+        Source source = source(ctx);
 
+        if (ctx.disallowed != null && ctx.sequenceParams() != null) {
+            throw new ParsingException(source, "Please specify sequence [by] before [with] not after");
+        }
+
+        List<Expression> parentJoinKeys = visitJoinKeys(ctx.by);
         TimeValue maxSpan = visitSequenceParams(ctx.sequenceParams());
 
         LogicalPlan until;
@@ -122,7 +127,7 @@ public abstract class LogicalPlanBuilder extends ExpressionBuilder {
             until = visitSequenceTerm(ctx.until, parentJoinKeys);
         } else {
             // no until declared means the condition never gets executed and thus folds to false
-            until = new Filter(source(ctx), RELATION, new Literal(source(ctx), Boolean.FALSE, DataTypes.BOOLEAN));
+            until = new Filter(source, RELATION, new Literal(source(ctx), Boolean.FALSE, DataTypes.BOOLEAN));
         }
 
         int numberOfKeys = -1;
@@ -145,7 +150,7 @@ public abstract class LogicalPlanBuilder extends ExpressionBuilder {
             }
         }
 
-        return new Sequence(source(ctx), queries, until, maxSpan);
+        return new Sequence(source, queries, until, maxSpan);
     }
 
     public KeyedFilter visitSequenceTerm(SequenceTermContext ctx, List<Expression> joinKeys) {
@@ -173,45 +178,40 @@ public abstract class LogicalPlanBuilder extends ExpressionBuilder {
             }
             
             String timeString = text(ctx.timeUnit().IDENTIFIER());
-            TimeUnit timeUnit = TimeUnit.SECONDS;
+            
-            if (timeString != null) {
+            if (timeString == null) {
+                throw new ParsingException(source(ctx.timeUnit()), "No time unit specified, did you mean [s] as in [{}s]?", text(ctx
+                        .timeUnit()));
+            }
+            
+            TimeUnit timeUnit = null;
             switch (timeString) {
-                    case "":
+                case "ms":
+                    timeUnit = TimeUnit.MILLISECONDS;
+                    break;
                 case "s":
-                    case "sec":
-                    case "secs":
-                    case "second":
-                    case "seconds":
                     timeUnit = TimeUnit.SECONDS;
                     break;
                 case "m":
-                    case "min":
-                    case "mins":
-                    case "minute":
-                    case "minutes":
                     timeUnit = TimeUnit.MINUTES;
                     break;
                 case "h":
-                    case "hs":
-                    case "hour":
-                    case "hours":
                     timeUnit = TimeUnit.HOURS;
                     break;
                 case "d":
-                    case "ds":
-                    case "day":
-                    case "days":
                     timeUnit = TimeUnit.DAYS;
                     break;
                 default:
-                        throw new ParsingException(source(ctx.timeUnit().IDENTIFIER()), "Unrecognized time unit [{}]", timeString);
+                    throw new ParsingException(source(ctx.timeUnit().IDENTIFIER()),
-                }
+                            "Unrecognized time unit [{}] in [{}], please specify one of [ms, s, m, h, d]", 
+                            timeString, text(ctx.timeUnit()));
             }
 
             return new TimeValue(value, timeUnit);
 
         } else {
-            throw new ParsingException(source(numberCtx), "Decimal time interval [{}] not supported yet", text(numberCtx));
+            throw new ParsingException(source(numberCtx), "Decimal time interval [{}] not supported; please use an positive integer",
+                    text(numberCtx));
         }
     }
 }

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/planner/QueryFolderFailTests.java

@@ -20,9 +20,9 @@ public class QueryFolderFailTests extends AbstractQueryFolderTestCase {
     }
 
     private String errorParsing(String eql) {
-        ParsingException e = expectThrows(ParsingException.class, () -> plan(eql));
+        Exception e = expectThrows(Exception.class, () -> plan(eql));
+        assertTrue(e.getClass().getSimpleName().endsWith("ParsingException"));
         final String header = "line ";
-        assertTrue(e.getMessage().startsWith(header));
         return e.getMessage().substring(header.length());
     }
 
@@ -187,4 +187,14 @@ public class QueryFolderFailTests extends AbstractQueryFolderTestCase {
         assertEquals("Found 1 problem\n" +
                 "line 1:15: first argument of [wildcard(pid, '*.exe')] must be [string], found value [pid] type [long]", msg);
     }
+    
+    public void testSequenceWithBeforeBy() {
+        String msg = errorParsing("sequence with maxspan=1s by key [a where true] [b where true]");
+        assertEquals("1:2: Please specify sequence [by] before [with] not after", msg);
+    }
+
+    public void testSequenceWithNoTimeUnit() {
+        String msg = errorParsing("sequence with maxspan=30 [a where true] [b where true]");
+        assertEquals("1:24: No time unit specified, did you mean [s] as in [30s]?", msg);
+    }
 }

x-pack/plugin/eql/src/test/resources/queries-supported.eql

@@ -546,7 +546,7 @@ sequence by unique_pid
     [network where true]
 ;
 
-sequence by pid with maxspan=200 
+sequence by pid with maxspan=200s
     [process where process_name == "*" ] 
     [file where file_path == "*"]
 ;
@@ -556,12 +556,12 @@ sequence by pid with maxspan=2s
     [file where file_path == "*"]
 ;
 
-sequence by pid with maxspan=2sec 
+sequence by pid with maxspan=500ms 
     [process where process_name == "*" ] 
     [file where file_path == "*"]
 ;
 
-sequence by pid with maxspan=2seconds
+sequence by pid with maxspan=2h
     [process where process_name == "*" ] 
     [file where file_path == "*"]
 ;