EQL: implement stringContains function by aleksmaus · Pull Request #54380 · elastic/elasticsearch

aleksmaus · 2020-03-28T19:04:45Z

Related to #54136

elasticmachine · 2020-03-28T19:04:48Z

Pinging @elastic/es-ql (:Query Languages/EQL)

rw-access · 2020-03-30T15:44:33Z

...va/org/elasticsearch/xpack/eql/expression/function/scalar/stringcontains/StringContains.java

+
+    private final Expression haystack, needle, caseSensitive;
+
+    public StringContains(Source source, Expression haystack, Expression needle, Expression caseSensitive) {


I think (per a slack conversation) that we'll handle case-sensitivity separately, so no need for the argument?

yeah, was not clear on this last week, so left it here for now. will remove as needed.

Removed the optional parameter.

astefan

Left some comments.

astefan · 2020-03-30T17:41:34Z

...va/org/elasticsearch/xpack/eql/expression/function/scalar/stringcontains/StringContains.java

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+package org.elasticsearch.xpack.eql.expression.function.scalar.stringcontains;


Package I think should be org.elasticsearch.xpack.eql.expression.function.scalar.string.

Moved to the scalar.string package.

astefan · 2020-03-30T17:44:49Z

...va/org/elasticsearch/xpack/eql/expression/function/scalar/stringcontains/StringContains.java

+
+    private final Expression haystack, needle, caseSensitive;
+
+    public StringContains(Source source, Expression haystack, Expression needle, Expression caseSensitive) {


Personally, I find haystack and needle inappropriate here. I know that it fits with the purpose of the function :-), but I prefer string and pattern or substring.

http://man7.org/linux/man-pages/man3/strstr.3.html :-)

Renamed params to string and substring.

astefan · 2020-03-30T17:48:08Z

...va/org/elasticsearch/xpack/eql/expression/function/scalar/stringcontains/StringContains.java

+        this.caseSensitive = arguments().get(2);
+    }
+
+    private static Expression toDefault(Expression exp) {


I'm not convinced this method is really needed. It's just a short one-liner... Maybe other reviewers thing otherwise, but I would keep using the ternary operator inline.

Removed the method since no more optional param.

astefan · 2020-03-30T17:49:50Z

...icsearch/xpack/eql/expression/function/scalar/stringcontains/StringContainsFunctionPipe.java

+    private final Pipe haystack, needle, caseSensitive;
+
+    public StringContainsFunctionPipe(Source source, Expression expression, Pipe haystack, Pipe needle, Pipe caseSensitive) {
+        super(source, expression, Arrays.asList(haystack, needle));


No caseSensitive?

Removed caseSensitive param completely.

astefan · 2020-03-30T17:50:48Z

...rch/xpack/eql/expression/function/scalar/stringcontains/StringContainsFunctionProcessor.java

+
+public class StringContainsFunctionProcessor implements Processor {
+
+    public static final String NAME = "sstringcontains";


sstc maybe?

Updated the NAME

astefan · 2020-03-30T17:52:35Z

...g/elasticsearch/xpack/eql/expression/function/scalar/stringcontains/StringContainsUtils.java

+
+import static org.elasticsearch.common.Strings.hasLength;
+
+final class StringContainsUtils {


Why this class? Shouldn't this method belong to a class that has other String utility method like this one here? And is this really needed to be placed in a separate class?

I think these methods could be in function.scalar.string.StringUtils

Moved the method to function.scalar.string.StringUtils.

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

aleksmaus · 2020-04-04T18:53:53Z

@elasticmachine run elasticsearch-ci/2

rw-access · 2020-04-06T16:38:59Z

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

+
+/**
+ * EQL specific stringContains function.
+ * https://eql.readthedocs.io/en/latest/query-guide/functions.html#stringContains


we can probably remove this link since Elasticsearch will eventually be the authoritative documentation

rw-access · 2020-04-06T16:39:35Z

...asticsearch/xpack/eql/expression/function/scalar/string/StringContainsFunctionProcessor.java

+    }
+
+    public static Object doProcess(Object string, Object substring) {
+        if (string == null) {


Suggested change

if (string == null) {

if (string == null || substring == null) {

throwing exception for now if the second param is null, consistent across all 3 functions that I implemented so far

are we leaning towards returning nulls more often and not throwing exceptions?

rw-access · 2020-04-06T16:40:47Z

...org/elasticsearch/xpack/eql/expression/function/scalar/whitelist/InternalEqlScriptUtils.java

        return (String) SubstringFunctionProcessor.doProcess(s, start, end);
    }
+
+    public static Boolean stringContains(String string, String substring) {


before substring for alphabetical order

rw-access · 2020-04-06T16:41:50Z

x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt

 "params":{"v0":"file_name.keyword","v1":-4,"v2":null,"v3":".exe"}


+stringContains


between startswith and substring?

astefan

LGTM. Left few minor comments.

astefan · 2020-04-06T23:01:21Z

...search/xpack/eql/expression/function/scalar/string/StringContainsFunctionProcessorTests.java

+                    assertThat(e.getMessage(), equalTo("A string/char is required; received [null]"));
+            } else {
+                assertThat(StringContainsFunctionProcessor.doProcess(string, substring),
+                        equalTo(string == null? null : true));


string == null ? null : true

astefan · 2020-04-06T23:05:04Z

...est/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/StringUtilsTests.java

+    protected static final int NUMBER_OF_TEST_RUNS = 20;
+
+    private static void run(Callable<Void> callable) throws Exception {
+        for (int runs = 0; runs < NUMBER_OF_TEST_RUNS; runs++) {


Imo, I don't think we need this high degree of unit tests for this particular functionality, but wait for @costin's take on this before modifying this or leaving it as is.

astefan · 2020-04-06T23:05:32Z

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

+
+    private String error(String query) {
+        VerificationException e = expectThrows(VerificationException.class,
+                () -> plan(query));


I think this line fits inside the previous one.

* EQL: implement stringContains function * Address code review comments * Fix javadoc comment for stringContains function * Address code review comments * Adjust queryfolder_tests.txt to the new format

EQL: implement stringContains function

c01c093

aleksmaus added >feature :Analytics/EQL EQL querying labels Mar 28, 2020

aleksmaus requested review from astefan, costin and rw-access March 28, 2020 19:04

aleksmaus requested a review from imotov March 28, 2020 19:04

rw-access reviewed Mar 30, 2020

View reviewed changes

astefan reviewed Mar 30, 2020

View reviewed changes

rw-access reviewed Mar 30, 2020

View reviewed changes

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

aleksmaus added 3 commits April 4, 2020 09:37

Merge branch 'master' into feature/stringContains

ed9f799

Address code review comments

8472a00

Fix javadoc comment for stringContains function

4e6adef

aleksmaus requested review from astefan and rw-access April 6, 2020 12:54

rw-access reviewed Apr 6, 2020

View reviewed changes

Address code review comments

e19ba67

astefan approved these changes Apr 6, 2020

View reviewed changes

aleksmaus added 3 commits April 7, 2020 13:05

Merge branch 'master' into feature/stringContains

6d25967

Merge branch 'master' into feature/stringContains

dd07556

Adjust queryfolder_tests.txt to the new format

65765cc

aleksmaus merged commit 3cd858c into elastic:master Apr 7, 2020

aleksmaus mentioned this pull request Apr 7, 2020

EQL: implement stringContains function (#54380) #54923

Merged

aleksmaus added a commit that referenced this pull request Apr 7, 2020

EQL: implement stringContains function (#54380) (#54923)

de38127

jrodewig mentioned this pull request Apr 8, 2020

[DOCS] EQL: Document stringContains function #54968

Merged


		private final Expression haystack, needle, caseSensitive;

		public StringContains(Source source, Expression haystack, Expression needle, Expression caseSensitive) {


		public class StringContainsFunctionProcessor implements Processor {

		public static final String NAME = "sstringcontains";


		import static org.elasticsearch.common.Strings.hasLength;

		final class StringContainsUtils {

	if (string == null) {
	if (string == null \|\| substring == null) {

		"params":{"v0":"file_name.keyword","v1":-4,"v2":null,"v3":".exe"}


		stringContains

Conversation

aleksmaus commented Mar 28, 2020

Uh oh!

elasticmachine commented Mar 28, 2020

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

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!

Uh oh!

aleksmaus commented Apr 4, 2020

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!

aleksmaus Apr 6, 2020 • 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!

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!

aleksmaus Apr 6, 2020 •

edited

Loading