EQL: Introduce ~ grammar for case-insensitive functions #67869

costin · 2021-01-24T16:10:51Z

By default, operators and string comparisons are case sensitive. This PR
enables support for case insensitive comparisons through the ~ (tilde)
symbol which works on functions.

For example:
process where ~endsWith(file.path, "ExE")

will match all file.path that end with "exe", regardless of their case.

In the process, refactored the code by introducing a common class and
improving code consistency.

Fix #67868

By default, operators and string comparisons are case sensitive. This PR enables support for case insensitive comparisons through the ~ (tilde) symbol which works on functions. For example: process where ~endsWith(file.path, "ExE") will match all file.path that end with "exe", regardless of their case. In the process, refactored the code by introducing a common class and improving code consistency. Fix elastic#67868

elasticmachine · 2021-01-24T16:10:54Z

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

costin · 2021-01-24T16:17:18Z

This PR is a doozy - I realize it's big and so was working on it :)
The new functionality lies in extending EqlFunctionRegistry to support case insensitive functions which are tagged in the grammar. In the proper however I've ended up refactoring a bit both the functions and their tests since there was significant repetition between them.
In the process I've changed the case sensitivity parameter from case sensitive to case insensitive to align it with the underlying ES/Lucene conventions and some inconsistencies since some functions had it one way, others the opposite.
The tests, both integration and unit have been enabled and in the process realized some were simply incorrect so these had to be updated.
Due to the size and the number of files touched this PR might not address all functionality around case-insensitive functions; I'd rather have a thorough follow-up from somebody else not just around integration testing but also around EQL benchmarks.

rw-access · 2021-01-24T16:17:37Z

Is this the right syntax? I thought we were leaning towards ~ as a suffix not a prefix. i.e. startswith~(process.name, "net.")

costin · 2021-01-24T16:33:25Z

Hi @rw-access

You are right, now that you mention it I recall that was the outcome. The prefix worked it way as I was modifying the queries and due to ease of use, the ~ ended as a prefix instead of a suffix.
I'll update the grammar latter on.

astefan

I've left some comments.

astefan · 2021-01-25T14:56:11Z

...ql/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/EndsWith.java

-
-        return isStringAndExact(pattern, sourceText(), ParamOrdinal.SECOND);
+    public EndsWith(Source source, Expression input, Expression pattern, boolean caseInsensitive) {
+        super(source, input, pattern, caseInsensitive);


The name of the fourth parameter here - caseInsensitive - is in contradiction with the name of the parameter in the superclass' constructor.

astefan · 2021-01-25T16:03:25Z

...ql/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/EndsWith.java

-    @Override
-    public boolean foldable() {
-        return input.foldable() && pattern.foldable();
+        return new EndsWithFunctionPipe(source(), this, Expressions.pipe(left()), Expressions.pipe(right()), isCaseInsensitive());


This class is not present in this PR, but EndsWithFunctionPipe has isCaseSensitive as a case sensitivity flag. From what I've seen in the PR, you make a case about case-insensitivity. Unless I'm missing something, it would be better if the case sensitivity aspect is consistent throughout the code base (naming wise). Meaning, change isCaseSensitive to isCaseInsensitive everywhere in code.
Also, the following classes still have the same caseSensitive parameter:

StringContains

StringContainsFunctionPipe

BetweenFunctionProcessor

astefan · 2021-01-25T16:24:15Z

...eql/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/Between.java

+    }
+
+    public Expression right() {
+        return input;


Suggested change

return input;

return right;

astefan · 2021-01-25T16:37:38Z

.../org/elasticsearch/xpack/eql/expression/function/scalar/string/BetweenFunctionProcessor.java

@@ -17,22 +19,29 @@

    public static final String NAME = "sbtw";

-    private final Processor input, left, right, greedy, caseSensitive;
+    private final Processor input, left, right, greedy;
+    private final boolean isCaseSensitive;


The naming of this variable is also inconsistent throughout the code from a different angle: here you have isCaseSensitive while in other places (for example BetweenFunctionPipe) it's caseInsensitive (is is used in some places while not used in others).

...eql/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/Between.java

costin · 2021-01-25T19:20:41Z

@elasticmachine update branch

bpintea

LGTM.

bpintea · 2021-01-26T10:24:54Z

x-pack/plugin/eql/qa/common/src/main/resources/test_queries.toml

 '''
 expected_event_ids  = [88, 92]
 description = "check built-in string functions"

 [[queries]]
 name = "startsWithCaseInsensitive3"
 query = '''
-file where opcode==0 and startsWith(file_name, "expLORER.exe")
+file where opcode==0 and startsWith~(file_name, "expLORER.exe")


Not introduced by this PR, but: is this test bringing anything new to startsWithCaseInsensitive2 ? If so, maybe the name should reflect that.

The previous query checks explorer., the second explorer.exe

Both startsWithCaseInsensitive2 and startsWithCaseInsensitive3 test mid-word capitals, but I guess the real difference is that one contains the full word and the other doesn't, which was probably the intend to have them separately. All good.
Edit: so much more as it's been updated :-)

bpintea · 2021-01-26T10:29:10Z

...eql/src/main/java/org/elasticsearch/xpack/eql/expression/function/EqlFunctionDefinition.java

+    @Override
+    protected Builder builder() {
+        return super.builder();
+    }


Is this needed?

Yes, the parent method is protected and is not visible outside classes in the same package - that is nobody outside the package in ql has access to it. By overriding it, it becomes accessible in the eql package as well.

bpintea · 2021-01-26T10:40:23Z

...n/eql/src/main/java/org/elasticsearch/xpack/eql/expression/function/EqlFunctionRegistry.java

+        return def(function, builder, names);
+    }
+
+    private static Boolean defaultSensitivity(Boolean caseInsensitive) {


Nit: my implementation-not-yet-read interpretation is that the function returns the default sensitivity (a constant). Smth in the lines of sensitivityOrDefault(...) might arguably be clearer.

bpintea · 2021-01-26T10:57:41Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/QueryTranslator.java

+                        wildcardQuery = "*" + string + "*";
+                    } else if (f instanceof EndsWith) {
+                        wildcardQuery = "*" + string;


Not sure if feasible by now (if already mostly used like that in the codebase), but extracting the wildcard character into a constant might make it cleaner.

The String is already internalized by the JVM however I've extracted inside the utils class since we use it there.
The issue with extracting constants is that if they aren't used consistently, they just occupy space.

bpintea · 2021-01-26T11:04:03Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/gen/script/Scripts.java

@@ -33,19 +34,22 @@
    public static final String INTERNAL_EQL_SCRIPT_UTILS = "InternalEqlScriptUtils";
    public static final String INTERNAL_SQL_SCRIPT_UTILS = "InternalSqlScriptUtils";

-    private Scripts() {}
+    private static final int PKG_LENGTH = "org.elasticsearch.xpack.".length();


palesz

A couple of concerns about deser to/from StreamInput/Output, otherwise only a few nitpicks and minor comments.

palesz · 2021-01-26T14:23:21Z

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

+TILDE_IDENTIFIER
+    : LETTER (LETTER | DIGIT | '_')* '~'
+    ;
+


Nit: I expected that the functionName will be IDENTIFIER '~'?. Why are function names allowed to start with _ and @ only if the function is not suffixed with ~?

I guess you don't want to allow ~ for @timestamp, but what about functions starting with _?

No. Defining a new token just for function names would overlap with the IDENTIIFER and the grammar ends up applying whoever is declared first due to ambiguity leading to rules not matching and thus broken queries.

palesz · 2021-01-26T14:30:47Z

...eql/src/main/java/org/elasticsearch/xpack/eql/expression/function/EqlFunctionResolution.java

+public enum EqlFunctionResolution implements FunctionResolutionStrategy {
+
+    /**
+     * Indicate the function should be insensitive {@code ~startwith()},


Suggested change

* Indicate the function should be insensitive {@code ~startwith()},

* Indicate the function should be insensitive {@code startwith~()},

palesz · 2021-01-26T14:52:22Z

x-pack/plugin/eql/src/test/resources/test_string_functions.toml

+    [[endswith_sensitive.fold.tests]]
+    expression = 'endsWith~("FooBarBaz", "baz")'
+    expected = true
+


Shouldn't this be false?

No. The naming is misleading (sensitive as in sensitivity not case sensitive vs insensitive) due to how the spec file was created - which should be changed.

palesz · 2021-01-26T16:36:19Z

.../org/elasticsearch/xpack/eql/expression/function/scalar/string/BetweenFunctionProcessor.java

+            caseInsensitive = in.readBoolean();
+        } else {
+            in.readNamedWriteable(Processor.class);
+            caseInsensitive = true;


Before it was caseSensitive = Boolean.TRUE:

elasticsearch/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/Between.java

Line 48 in c5c3bee

this.caseSensitive = Literal.TRUE;

Shouldn't this be:

Suggested change

caseInsensitive = true;

caseInsensitive = false;

No, before it was caseSensitive true not its caseInsensitive false. Please see again the main comments made on the issue.

palesz · 2021-01-26T16:42:50Z

...org/elasticsearch/xpack/eql/expression/function/scalar/string/EndsWithFunctionProcessor.java

    }

    public EndsWithFunctionProcessor(StreamInput in) throws IOException {
        input = in.readNamedWriteable(Processor.class);
        pattern = in.readNamedWriteable(Processor.class);
-        isCaseSensitive = in.readBoolean();
+        isCaseInsensitive = in.readBoolean();


Should you do a version check here similar to the BetweenFunctionProcessor? Meaning of the boolean flipped between 7.11 -> 7.12.

No, that was actually a leftover. The processors in EQL have serialization which we use in testing but there's no serialization on the client.
The SQL ones use it for the cursor however we offer no guarantees, as of yet, on allowing scrolling across nodes with different SQL versions (essentially a rolling upgrade).

palesz · 2021-01-26T17:38:38Z

.../src/test/java/org/elasticsearch/xpack/eql/expression/function/EqlFunctionRegistryTests.java

+                return new DummyFunction(l);
+            }, "DUMMY_FUNCTION");
+        FunctionRegistry r = new EqlFunctionRegistry(definition);
+        FunctionDefinition def = r.resolveFunction(ur.name());


Missing: assertEquals(ur.source(), ur.buildResolved(randomConfiguration(), def).source());

palesz · 2021-01-26T17:40:51Z

.../src/test/java/org/elasticsearch/xpack/eql/expression/function/EqlFunctionRegistryTests.java

+        assertThat(e.getMessage(), endsWith("expects exactly two arguments"));
+    }
+
+    public void testTernaryCaseAwareWithOptionalFunction() {


This test case needs an overhaul, it doesn't really test what the name suggests and half of the asserts are never tested.

palesz · 2021-01-26T18:01:03Z

...rg/elasticsearch/xpack/ql/expression/function/scalar/string/StartsWithFunctionProcessor.java

    public StartsWithFunctionProcessor(StreamInput in) throws IOException {
        source = in.readNamedWriteable(Processor.class);
        pattern = in.readNamedWriteable(Processor.class);
-        isCaseSensitive = in.readBoolean();
+        caseInsensitive = in.readBoolean();
    }

    @Override
    public final void writeTo(StreamOutput out) throws IOException {
        out.writeNamedWriteable(source);
        out.writeNamedWriteable(pattern);
-        out.writeBoolean(isCaseSensitive);
+        out.writeBoolean(caseInsensitive);


Same as above.

palesz · 2021-01-26T18:01:16Z

...rg/elasticsearch/xpack/ql/expression/function/scalar/string/StartsWithFunctionProcessor.java

@@ -60,13 +60,13 @@ public static Object doProcess(Object source, Object pattern, boolean isCaseSens
            throw new QlIllegalArgumentException("A string/char is required; received [{}]", pattern);
        }

-        if (isCaseSensitive) {
+        if (caseInsensitive == false) {


Flip the if.

palesz · 2021-01-26T22:13:20Z

.../org/elasticsearch/xpack/eql/expression/function/scalar/string/BetweenFunctionPipeTests.java

+            randomStringLiteral(),
+            randomStringLiteral(),
+            randomStringLiteral(),
+            randomStringLiteral(),


Minor: why did the greedy param change to randomStringLIteral?

matriv

LGTM

matriv · 2021-01-26T13:45:45Z

...eql/src/main/java/org/elasticsearch/xpack/eql/expression/function/EqlFunctionResolution.java

+public enum EqlFunctionResolution implements FunctionResolutionStrategy {
+
+    /**
+     * Indicate the function should be insensitive {@code ~startwith()},


Suggested change

* Indicate the function should be insensitive {@code ~startwith()},

* Indicate the function should be insensitive {@code startwith~()},

matriv · 2021-01-26T13:47:30Z

...ql/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/EndsWith.java

-    @Override
-    public boolean foldable() {
-        return input.foldable() && pattern.foldable();
+        return new EndsWithFunctionPipe(source(), this, Expressions.pipe(left()), Expressions.pipe(right()), isCaseInsensitive());


matriv

LGTM

costin · 2021-01-28T21:32:14Z

@elasticmachine update branch

costin · 2021-01-28T22:32:46Z

/cc @jrodewig

By default, operators and string comparisons are case sensitive. This PR enables support for case insensitive comparisons through the ~ (tilde) symbol which works on functions. For example: process where endsWith~(file.path, "ExE") will match all file.path that end with "exe", regardless of their case. In the process, refactored the code by introducing a common class and improving code consistency. Fix #67868 (cherry picked from commit afa4ce0)

Documents the case-insensitive `~` operator for `in` and string functions. Relates to #67869 and #68176

costin added >feature v8.0.0 :Analytics/EQL EQL querying labels Jan 24, 2021

costin requested review from astefan, palesz, bpintea and matriv January 24, 2021 16:10

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

costin added the v7.12.0 label Jan 24, 2021

costin added 5 commits January 24, 2021 20:59

Fix constructor visibility

26fd397

Add back generic scalar function handling

419902b

Fix default in SQL StartsWith

8733f70

Fix default in SQL StartsWith

3917f37

Change ~ from prefix to suffix

0e65475

costin force-pushed the eql/tilde branch from c7b32fe to 0e65475 Compare January 24, 2021 20:50

astefan reviewed Jan 25, 2021

View reviewed changes

Address feedback

d8011c3

Merge branch 'master' into eql/tilde

f4c7abf

bpintea approved these changes Jan 26, 2021

View reviewed changes

Address feedback

e75f2a4

palesz suggested changes Jan 26, 2021

View reviewed changes

matriv approved these changes Jan 27, 2021

View reviewed changes

Address feedback

d1c2f11

Merge branch 'master' into eql/tilde

3495163

costin merged commit afa4ce0 into elastic:master Jan 28, 2021

costin deleted the eql/tilde branch January 28, 2021 22:32

jrodewig mentioned this pull request Jan 29, 2021

[DOCS] EQL: Add case-insensitive ~ operator #68217

Merged

jrodewig added a commit that referenced this pull request Jan 29, 2021

[DOCS] EQL: Add case-insensitive ~ operator (#68217)

ab3f8f5

Documents the case-insensitive `~` operator for `in` and string functions. Relates to #67869 and #68176

jrodewig added a commit that referenced this pull request Jan 29, 2021

[DOCS] EQL: Add case-insensitive ~ operator (#68217) (#68236)

4c041a1

Documents the case-insensitive `~` operator for `in` and string functions. Relates to #67869 and #68176

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	* Indicate the function should be insensitive {@code ~startwith()},
	* Indicate the function should be insensitive {@code startwith~()},

EQL: Introduce ~ grammar for case-insensitive functions #67869

EQL: Introduce ~ grammar for case-insensitive functions #67869

Uh oh!

Conversation

costin commented Jan 24, 2021

Uh oh!

elasticmachine commented Jan 24, 2021

Uh oh!

costin commented Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rw-access commented Jan 24, 2021

Uh oh!

costin commented Jan 24, 2021

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!

Uh oh!

costin commented Jan 25, 2021

Uh oh!

bpintea 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!

bpintea Jan 26, 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!

costin Jan 26, 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!

Choose a reason for hiding this comment

Uh oh!

palesz 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

costin commented Jan 24, 2021 •

edited

Loading

bpintea Jan 26, 2021 •

edited

Loading

costin Jan 26, 2021 •

edited

Loading