EQL: implement cidrMatch function by aleksmaus · Pull Request #54186 · elastic/elasticsearch

aleksmaus · 2020-03-25T13:48:42Z

Related to #54132

elasticmachine · 2020-03-25T13:48:44Z

Pinging @elastic/es-search (:Search/EQL)

aleksmaus · 2020-03-25T13:49:29Z

...k/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/CommonEqlActionTestCase.java

            return;
        }

+        CreateIndexRequest request = new CreateIndexRequest(testIndexName)


Needed mapping to IP type, this creating the index with mapping.

aleksmaus · 2020-03-25T13:50:40Z

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

+        for (Expression addr: addresses) {
+            // Currently we have limited enum for ordinal numbers
+            // So just using default here for error messaging
+            resolution = isStringAndExact(addr, sourceText(), ParamOrdinal.DEFAULT);


The current enum for error messages doesn't work great for variadic type of function. Open to suggestions.

I added a method here in my wildcard PR:

elasticsearch/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java

Lines 36 to 44 in d482c47

public static ParamOrdinal fromIndex(int index) {

switch (index) {

case 0: return ParamOrdinal.FIRST;

case 1: return ParamOrdinal.SECOND;

case 2: return ParamOrdinal.THIRD;

case 3: return ParamOrdinal.FOURTH;

default: return ParamOrdinal.DEFAULT;

}

}

Also, should these fields also be isStringAndExact? and an isFoldable check?
Here's how I approached that

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

Lines 80 to 83 in d482c47

lastResolution = isFoldable(p, sourceText(), ParamOrdinal.fromIndex(index));

if (lastResolution.unresolved()) {

break;

}

Cool! Will update after your PR merges.
The ParamOrdinal falls back to ParamOrdinal.DEFAULT after 4 params anyways. Maybe make enum longer if we still want to use enum there?

For how to handle functions that deal with a variable list of parameters, I suggest having a look at org.elasticsearch.xpack.sql.expression.predicate.conditional.Case or org.elasticsearch.xpack.sql.expression.predicate.conditional.Coalesce since these are functions that have a variable list of parameters.

Relying on the same def builder here that was merged with wildcard function for now.
Updated to align with wildcard impl usage of ParamOrdinal.

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

astefan

Left few comments.

astefan · 2020-03-26T13:09:50Z

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

        // String
            new FunctionDefinition[] {
                def(Substring.class, Substring::new, "substring"),
+                def(CIDRMatch.class, CIDRMatch::new, "cidrmatch"),


I prefer listing the functions in alphabetical order.

Yep. Updated.

astefan · 2020-03-26T13:12:25Z

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

+import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isStringAndExact;
+
+/**
+ * EQL specific cidrMatch function


Personal preference, but I would like to see a useful description of the function. Not even the original EQL documentation is clear imo: https://eql.readthedocs.io/en/latest/query-guide/functions.html

Added a couple of more lines with a link to the EQL doc.

astefan · 2020-03-26T13:20:47Z

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

+        for (Expression addr: addresses) {
+            // Currently we have limited enum for ordinal numbers
+            // So just using default here for error messaging
+            resolution = isStringAndExact(addr, sourceText(), ParamOrdinal.DEFAULT);


For how to handle functions that deal with a variable list of parameters, I suggest having a look at org.elasticsearch.xpack.sql.expression.predicate.conditional.Case or org.elasticsearch.xpack.sql.expression.predicate.conditional.Coalesce since these are functions that have a variable list of parameters.

aleksmaus · 2020-03-28T19:09:34Z

Will address the code review comments and make some adjustments once the wildcard function PR (#54020) is merged, since will have some conflicts and can reuse some code between the functions.

matriv · 2020-03-30T15:30:06Z

@elastic/es-ql

aleksmaus · 2020-04-04T20:43:26Z

@elasticmachine run elasticsearch-ci/docs

aleksmaus · 2020-04-04T21:11:42Z

@elasticmachine run elasticsearch-ci/2

costin

Looks good to me - left a comment re: #54795 (waiting for the build to pass before merging).
I would also like some tests regarding incorrect arguments - what if the given string is non-numeric or contains an invalid IP.
How does the current EQL implementation handle that - I'm fine with addressing this in a separate PR however I'd like to have an issue tracking this so it doesn't get lost.
That is, how do we validate and protect against invalid parameters.

costin · 2020-04-06T20:43:44Z

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

+        return new CIDRMatch(source(), newChildren.get(0), newChildren.subList(1, newChildren.size()));
+    }
+
+    public ScalarFunction asFunction() {


Please update to take into account #54795 - reduces the class and removes a separate optimizer rule.

Sure, will do once #54795 is merged.

astefan

LGTM

aleksmaus · 2020-04-07T21:17:42Z

@costin regarding validation if the string is a valid IP address the original EQL implementation does validation with regex matching etc. In our case I was not sure if we need to add any additional validation, but pass the string down to elasticsearch and let it match against the IP type of field or handle the error if the value can not be used for matching.
Correct me if I'm wrong.

Related to elastic#54132

Related to #54132

EQL: implement cidrMatch function

aff0389

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

aleksmaus requested review from astefan, costin, imotov and rw-access March 25, 2020 13:48

aleksmaus commented Mar 25, 2020

View reviewed changes

rw-access reviewed Mar 25, 2020

View reviewed changes

...l/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/CIDRMatch.java Outdated Show resolved Hide resolved

jrodewig mentioned this pull request Mar 25, 2020

[DOCS] EQL: Document cidrMatch function #54216

Merged

astefan reviewed Mar 26, 2020

View reviewed changes

Merge branch 'master' into feature/cidrmatch

ee76e7c

aleksmaus added 2 commits April 4, 2020 16:16

Merge branch 'master' into feature/cidrmatch

439cbc2

Address code review comments

16ff57d

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

costin approved these changes Apr 6, 2020

View reviewed changes

astefan approved these changes Apr 6, 2020

View reviewed changes

aleksmaus added 3 commits April 7, 2020 16:14

Merge branch 'master' into feature/cidrmatch

0e4280d

Merge branch 'master' into feature/cidrmatch

5056054

Convert to SurrogateFunction

b1342e7

aleksmaus merged commit 99b9a18 into elastic:master Apr 7, 2020

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

EQL: implement cidrMatch function (elastic#54186)

13f6ac6

Related to elastic#54132

aleksmaus mentioned this pull request Apr 7, 2020

EQL: implement cidrMatch function (#54186) #54928

Merged

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

EQL: implement cidrMatch function (#54186) (#54928)

d02f774

Related to #54132

	public static ParamOrdinal fromIndex(int index) {
	switch (index) {
	case 0: return ParamOrdinal.FIRST;
	case 1: return ParamOrdinal.SECOND;
	case 2: return ParamOrdinal.THIRD;
	case 3: return ParamOrdinal.FOURTH;
	default: return ParamOrdinal.DEFAULT;
	}
	}

	lastResolution = isFoldable(p, sourceText(), ParamOrdinal.fromIndex(index));
	if (lastResolution.unresolved()) {
	break;
	}

Conversation

aleksmaus commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Mar 25, 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!

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!

aleksmaus commented Mar 28, 2020

Uh oh!

matriv commented Mar 30, 2020

Uh oh!

aleksmaus commented Apr 4, 2020

Uh oh!

aleksmaus commented Apr 4, 2020

Uh oh!

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

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

aleksmaus commented Apr 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aleksmaus commented Mar 25, 2020 •

edited

Loading