EQL: Add wildcard function by rw-access · Pull Request #54020 · elastic/elasticsearch

rw-access · 2020-03-23T20:23:35Z

Resolves #53999

Added wildcard variadic function
Added variadic builder QL for single argument + variadic
Added method to wildcard function to convert to an Or of Likes, and updated Optimizer rule to uses that.
Using makePipe, doProcess, etc on the converted Or of Likes.

elasticmachine · 2020-03-23T20:25:11Z

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

costin

Left a round of comments

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

costin · 2020-03-23T21:39:09Z

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

+    }
+
+    public Wildcard(Source source, Expression field, List<Expression> patterns) {
+        super(source, getArguments(field, patterns));


Suggested change

super(source, getArguments(field, patterns));

super(source, Arrays.asList(field, patterns));

like the rest of the subclasses.

doesn't seem to work if the first value is a T, and the next is a List<T>

CollectionUtils.combine(patterns, field) or if you want to preserve the order:
CollectionUtils.combine(singletonList(field), patterns)

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

costin · 2020-03-23T22:14:36Z

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

+ *     wildcard(field, "*wildcard*pattern*", "*wildcard*pattern*")
+ */
+
+public class Wildcard extends ScalarFunction {


Please implement the methods in the order of the super class (which is not alphabetical) which roughly is:

constructor
nodeInfo/replaceChildren
type resolution
getters
datatype/nullable
foldable/fold
scripting & co
equals/hash

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

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/utils/StringUtils.java

costin · 2020-03-23T22:28:46Z

...plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/FunctionRegistry.java

    }
+
+    @SuppressWarnings("overloads")  // These are ambiguous if you aren't using ctor references but we always do
+    public static <T extends Function> FunctionDefinition def(Class<T> function,


How many arguments does wildcard expect?

at least two, but it's unbounded in the maximum number
https://eql.readthedocs.io/en/latest/query-guide/functions.html#wildcard

...in/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/BinaryLogic.java

astefan

Left some comments. Some of these are probably because of my lack of understanding of how wildcard should work and what the scenarios it's covering.

astefan · 2020-03-24T15:21:11Z

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

                def(Substring.class, Substring::new, "substring"),
            },
+            new FunctionDefinition[] {
+                def(Wildcard.class, Wildcard::new, "wildcard"),


Isn't wildcard a "string function"? If so, it should belong to the FunctionDefinition array that, also, has substring in it. In SQL we were grouping these functions by their type: string, grouping, math, conditional, date etc.

oh right, good catch

astefan · 2020-03-24T15:23:16Z

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

+
+    @Override
+    protected TypeResolution resolveType() {
+        if (!childrenResolved()) {


childrenResolved() == false

astefan · 2020-03-24T15:29:36Z

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

+
+    @Override
+    public boolean foldable() {
+        return Expressions.foldable(arguments());


Shouldn't the field be, also, foldable? (ie return Expressions.foldable(children());)

Since wildcard is converted to a bunch of LIKEs, I'm wondering if foldable() shouldn't fall back to the result of the wildcard -> LIKEs transformation foldable() functionality. Basically the Or.foldable().

arguments() = field + patterns()
arguments() comes from the super and is identical to children()

I'll swap to children() since that's more obvious

astefan · 2020-03-24T16:31:28Z

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

+        }
+
+        for (Expression p: patterns) {
+            lastResolution = isStringAndExact(p, sourceText(), ParamOrdinal.DEFAULT);


I don't think isStringAndExact is correct here... "exact" refers to a field being of type keyword or having a sub-field of type keyword basically. isString should be enough imo.
Also, shouldn't the p.foldable() == false (comparison against variables basically) check be before this one?

astefan · 2020-03-24T16:45:23Z

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

+        return result;
+    }
+
+    private static List<Expression> toArguments(Expression src, List<Expression> patterns) {


Maybe move this method to org.elasticsearch.xpack.ql.util.CollectionUtils and make it more generic?

saw this kind fo construct used in few places

CollectionUtils.combine(singletonList(src), patterns))

astefan · 2020-03-24T16:46:58Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java

+                if (e instanceof Wildcard) {
+                    e = ((Wildcard) e).asLikes();
+                }
+
+                return e;


return e instanceof Wildcard ? ((Wildcard) e).asLikes() : e; as a shorter (hopefully more elegant) variant?

astefan · 2020-03-24T17:04:51Z

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

        assertEquals("Line 1:52: Comparisons against variables are not (currently) supported; offender [parent_process_name] in [==]", msg);
    }
+
+    public void testWildcardNotEnoughArguments() {


I think there are other error messages to check with wildcard: the fact that the field needs to be string and exact and, also, that the "patterns" should be all strings, no?

costin

LGTM

matriv · 2020-03-30T15:29:35Z

@elastic/es-ql

* EQL: Add wildcard function * EQL: Cleanup Wildcard.getArguments * EQL: Cleanup Wildcard and rearrange methods * EQL: Wildcard newline lint * EQL: Make StringUtils function final * EQL: Make Wildcard.asLikes return ScalarFunction * QL: Restore BinaryLogic.java * EQL: Add Wildcard PR feedback * EQL: Add Wildcard verification tests * EQL: Switch wildcard to isFoldable test * EQL: Change wildcard test to numeric field * EQL: Remove Wildcard.get_arguments

rw-access · 2020-04-03T16:19:04Z

Backport 022f829

EQL: Add wildcard function

c0c7995

rw-access added the :Analytics/EQL EQL querying label Mar 23, 2020

rw-access requested review from astefan and costin March 23, 2020 20:23

rw-access mentioned this pull request Mar 23, 2020

EQL: Add support for existing functions #51556

Closed

18 tasks

costin reviewed Mar 23, 2020

View reviewed changes

rw-access added 5 commits March 23, 2020 16:59

EQL: Cleanup Wildcard.getArguments

dea1f7b

EQL: Cleanup Wildcard and rearrange methods

d87361d

EQL: Wildcard newline lint

3826eac

EQL: Make StringUtils function final

f93d297

EQL: Make Wildcard.asLikes return ScalarFunction

78f4fbd

jrodewig mentioned this pull request Mar 24, 2020

[DOCS] EQL: Document wildcard function #54086

Merged

rw-access added 2 commits March 24, 2020 09:02

Merge remote-tracking branch 'origin/master' into eql/wildcard-function

c93b67c

QL: Restore BinaryLogic.java

169245d

astefan reviewed Mar 24, 2020

View reviewed changes

rw-access added 6 commits March 24, 2020 14:13

Merge branch 'master' into eql/wildcard-function

5c7e147

EQL: Add Wildcard PR feedback

e12a60c

EQL: Add Wildcard verification tests

502ca88

EQL: Switch wildcard to isFoldable test

90dc8e0

EQL: Change wildcard test to numeric field

d482c47

EQL: Remove Wildcard.get_arguments

285582f

aleksmaus mentioned this pull request Mar 28, 2020

EQL: implement cidrMatch function #54186

Merged

costin approved these changes Mar 30, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into eql/wildcard-function

167c13a

rw-access requested a review from aleksmaus March 30, 2020 20:22

Merge branch 'master' into eql/wildcard-function

d3b1dd1

rw-access requested a review from astefan April 2, 2020 14:42

aleksmaus approved these changes Apr 2, 2020

View reviewed changes

rw-access merged commit f04fb68 into elastic:master Apr 3, 2020

rw-access deleted the eql/wildcard-function branch April 3, 2020 16:14

	super(source, getArguments(field, patterns));
	super(source, Arrays.asList(field, patterns));

Conversation

rw-access commented Mar 23, 2020

Uh oh!

elasticmachine commented Mar 23, 2020

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rw-access Mar 23, 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

rw-access Mar 24, 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!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

matriv commented Mar 30, 2020

Uh oh!

rw-access commented Apr 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rw-access Mar 23, 2020 •

edited

Loading

rw-access Mar 24, 2020 •

edited

Loading