EQL: implement between function by aleksmaus · Pull Request #54277 · elastic/elasticsearch

aleksmaus · 2020-03-26T14:47:40Z

Related to #54135

elasticmachine · 2020-03-26T14:47:42Z

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

…ore tests.

astefan

Left few comments.

astefan · 2020-03-30T16:42:50Z

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

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


Why this package name? :-)
Shouldn't this be the same as for other string functions? org.elasticsearch.xpack.eql.expression.function.scalar.string

Yep. Will move everything under scalar.string package.

astefan · 2020-03-30T16:50:57Z

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

+
+public class BetweenFunctionProcessor implements Processor {
+
+    public static final String NAME = "sbetween";


I think sbtw is shorter and serves the same purpose.

astefan · 2020-03-30T17:04:58Z

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

        return (String) SubstringFunctionProcessor.doProcess(s, start, end);
    }
+
+    public static String between(String s, String left, String right, Boolean greedy, Boolean caseSensitive) {


Can you move this one before substring (to follow the alphabetical order), please?

astefan · 2020-03-30T17:05:13Z

x-pack/plugin/eql/src/main/resources/org/elasticsearch/xpack/eql/plugin/eql_whitelist.txt

 # ASCII Functions
 # 
  String  substring(String, Number, Number)
+  String  between(String, String, String, Boolean, Boolean)


Same here (alphabetical).

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

...java/org/elasticsearch/xpack/eql/expression/function/scalar/between/BetweenBaseTestCase.java

+
+import java.util.concurrent.Callable;
+
+public abstract class BetweenBaseTestCase extends ESTestCase {


I'll let other reviewers weigh in on the necessity of this approach here.

That isn't an usual approach with randomized testing. The Elasticsearch infra in CI does run the tests continuously and having a randomized value for parameters is usually enough.
A more common approach is to introduce randomness in tests with various random* methods and leave CI do its thing. A personal approach (that others I believe are following) is to stress test such a random test class with -Dtests.iters=1000 or such a large value before creating the PR. I'm not saying this is enough, but it gives one a greater degree of confidence over the committed code.

This seems to be the approach utilized by serializations tests, it's just extracted into the base class there, which we could consolidate across functions tests. Easy to remove. Let me know.

Removed the class and the multiple tests runs from randomized tests.

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

...t/java/org/elasticsearch/xpack/eql/expression/function/scalar/between/BetweenUtilsTests.java

+
+        assertThat(BetweenUtils.between("System Idle Process", "s", "e", false, false),
+                equalTo("yst"));
+    }


No test with last parameter set to true.

Good catch, thanks. Added more tests.

astefan · 2020-03-30T17:30:16Z

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


+
+    // Translation table for error messaging in the following function
+    private static final String[] NUM_NAMES = {


This should go somewhere at the top of this class.

Moved to the top of this class.

astefan · 2020-03-30T17:31:31Z

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

+            if (hasOptionalParams && (children.size() > NUM_TOTAL_PARAMS || children.size() < NUM_TOTAL_PARAMS - numOptionalParams)) {
+                throw new QlIllegalArgumentException("expects between " + NUM_NAMES[NUM_TOTAL_PARAMS - numOptionalParams]
+                        + " and " + NUM_NAMES[NUM_TOTAL_PARAMS] + " arguments");
+            } else if (!hasOptionalParams && children.size() != NUM_TOTAL_PARAMS) {


hasOptionalParams == false

astefan · 2020-03-30T17:39:03Z

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

+                throw new QlIllegalArgumentException("expects between " + NUM_NAMES[NUM_TOTAL_PARAMS - numOptionalParams]
+                        + " and " + NUM_NAMES[NUM_TOTAL_PARAMS] + " arguments");
+            } else if (!hasOptionalParams && children.size() != NUM_TOTAL_PARAMS) {
+                throw new QlIllegalArgumentException("expects exactly " + NUM_NAMES[NUM_TOTAL_PARAMS] + " arguments");


I understand this wants to be a generic approach for dealing with a variable number of optional parameters, but:

this solution needs to be generic throughout this class: here it's another method accepting optional parameters.

I don't understand the purpose of this method... it tries to be generic, but NUM_TOTAL_PARAMS is always 5. So, this method is for functions that can have between 0 and 5 optional parameters? Apart from between is there any other such function?

The approach by the here link in you comment allows only one optional parameter.
Here I created a kind of more generic implementation allowing to specify the number of optional parameters.
In the case of between function we need to support up to two optional parameters.

Let's revisit this based on whether between actually needs case sensitivity or not - if it doesn't it will simplify things.
Same applies for greedy.

/cc @rw-access

the greedy is not quite the same as case sensitivity, could be useful.

rw-access · 2020-04-01T20:50:07Z

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

@@ -0,0 +1,11 @@
+[[queries]]


why not just add these to test_queries_supported.toml?

Sure can rename the file and we could start using it for all the extra queries that are not included in the original test_queries.toml. Will do that.

Renamed the file into test_queries_supported.toml so we can add more queries there to avoid touching the original EQL test_queries.toml file.

rw-access · 2020-04-01T20:51:57Z

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

+    }
+
+    public static Object doProcess(Object source, Object left, Object right, Object greedy, Object caseSensitive) {
+        if (source == null) {


what if left and right are null?

Suggested change

if (source == null) {

if (source == null || left == null || right == null) {

Across functions that I was implementing so far I was assuming that we are returning null only if the source data (first param) is null.
The rest of params values should provided or there there will an exception, which is consistent with Java strings function overall that throw when the parameter is null.
Saw another approach was used by @astefan where was returning null if any of params are null.
Open for discussion, but would need to make consistent across of all of the functions.
Haven't heard a definite decision on this yet.

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

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

costin

Left a few comments otherwise LGTM

costin · 2020-04-06T20:49:42Z

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

+    }
+
+    private static void throwIfNotString(Object obj) {
+        if (!(obj instanceof String || obj instanceof Character)) {


It might be worth extracting these assertions into the Check class inside QL and reusing them through-out the code.
We've been going back and forth regarding this style - the if check is fairly simple but repeating the string is tedious (and typically tends to not be consistent).

yeah, I tried to avoid repetition of code and error strings. Moved these into QL Check class.

costin · 2020-04-06T20:50:34Z

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

+
+
+    @Override
+    public boolean equals(Object obj) {


Small nit - the getNameWritable tends to be towards the top after writeTo and hashCode before equals.

It looks like getWriteableName presently it is at the end of every FunctionProcessor so far.
Updated.

costin · 2020-04-06T20:51:40Z

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

+
+        int idx = matchString.indexOf(left);
+        if (idx == -1) {
+            return "";


You could return org.elasticsearch.xpack.ql.util.StringUtils.EMPTY instead.

costin · 2020-04-06T20:52:08Z

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


 package org.elasticsearch.xpack.eql.expression.function.scalar.string;

+import org.apache.directory.api.util.Strings;


Wrong import.

Good catch, thank you! Found another place in tests. Updated both places.

costin · 2020-04-06T20:52:30Z

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

+    }
+
+    // Test from EQL doc https://eql.readthedocs.io/en/latest/query-guide/functions.html
+    public void testBetweenBasicEQLExamples() {


costin · 2020-04-06T20:54:54Z

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

+                throw new QlIllegalArgumentException("expects between " + NUM_NAMES[NUM_TOTAL_PARAMS - numOptionalParams]
+                        + " and " + NUM_NAMES[NUM_TOTAL_PARAMS] + " arguments");
+            } else if (!hasOptionalParams && children.size() != NUM_TOTAL_PARAMS) {
+                throw new QlIllegalArgumentException("expects exactly " + NUM_NAMES[NUM_TOTAL_PARAMS] + " arguments");


Let's revisit this based on whether between actually needs case sensitivity or not - if it doesn't it will simplify things.
Same applies for greedy.

/cc @rw-access

costin · 2020-04-06T20:56:13Z

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

+    }
+
+    private static Expression toDefault(Expression exp) {
+        return exp != null ? exp : Literal.FALSE;


Better to create a new exception preservation the source - helps with tracking down the location of the argument:
new Literal(exp.source(), Boolean.FALSE, DataTypes.Boolean)

astefan

LGTM

costin · 2020-04-06T21:24:03Z

~~Could you please incorporate #54795? It should remove the need of an optimizer rule and make the function class smaller.~~

Scratch that - I mixed up the issues.

costin · 2020-04-07T11:49:01Z

x-pack/plugin/eql/qa/common/build.gradle

  compile project(':test:framework')
  compile project(path: xpackModule('core'), configuration: 'default')
  compile project(path: xpackModule('core'), configuration: 'testArtifacts')
+  compile project(path: xpackModule('ql'), configuration: 'default')


Since this import is just for CollectionUtils, I'd rather keep the projects separated and simply concatenate the lists through addAll.

Good point. Removed.

…e/between

* EQL: implement between function * Address WIP TODOs. Add more tests * Fix linter complaint * Add more query folder tests to cover invalid parameter types * Update Between toDefault. Fix typo in BetweenFunctionProcessor. Add more tests. * Remove unneeded null checks in BetweenFunctionProcessor::doProcess * Address code review comments * Address additional code review comments * Remove dependency on QL from eql/qa project

EQL: implement between function

354cf99

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

aleksmaus requested review from astefan, costin and rw-access March 26, 2020 14:47

Address WIP TODOs. Add more tests

341c725

aleksmaus removed the WIP label Mar 26, 2020

aleksmaus marked this pull request as ready for review March 26, 2020 20:01

aleksmaus added 5 commits March 26, 2020 16:11

Fix linter complaint

69c404f

Add more query folder tests to cover invalid parameter types

81cc366

Update Between toDefault. Fix typo in BetweenFunctionProcessor. Add m…

94ce5fa

…ore tests.

Remove unneeded null checks in BetweenFunctionProcessor::doProcess

61a7808

Merge branch 'master' into feature/between

be0b67c

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

astefan reviewed Mar 30, 2020

View reviewed changes

rw-access reviewed Apr 1, 2020

View reviewed changes

...ql/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/between/Between.java Show resolved Hide resolved

rw-access reviewed Apr 1, 2020

View reviewed changes

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

aleksmaus added 2 commits April 4, 2020 15:31

Merge branch 'master' into feature/between

779111c

Address code review comments

72265cd

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

Address additional code review comments

cff240e

Merge branch 'master' into feature/between

a0c9832

costin reviewed Apr 7, 2020

View reviewed changes

aleksmaus added 4 commits April 7, 2020 09:37

Remove dependency on QL from eql/qa project

d7ac4e8

Merge branch 'master' into feature/between

d6dfca5

Merge branch 'master' of github.com:elastic/elasticsearch into featur…

fd5d05a

…e/between

Merge branch 'master' into feature/between

39a329a

aleksmaus added the backport pending label Apr 7, 2020

aleksmaus merged commit b7f02d8 into elastic:master Apr 7, 2020

aleksmaus mentioned this pull request Apr 7, 2020

EQL: implement between function (#54277) #54913

Merged

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

EQL: implement between function (#54277) (#54913)

868798e

aleksmaus removed the backport pending label Apr 7, 2020

jrodewig mentioned this pull request Apr 8, 2020

[DOCS] EQL: Document between function #54950

Merged


		public class BetweenFunctionProcessor implements Processor {

		public static final String NAME = "sbetween";


		import java.util.concurrent.Callable;

		public abstract class BetweenBaseTestCase extends ESTestCase {



		// Translation table for error messaging in the following function
		private static final String[] NUM_NAMES = {

	if (source == null) {
	if (source == null \|\| left == null \|\| right == null) {


		package org.elasticsearch.xpack.eql.expression.function.scalar.string;

		import org.apache.directory.api.util.Strings;

Conversation

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

Uh oh!

Uh oh!

elasticmachine commented Mar 26, 2020

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!

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!

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!

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 26, 2020 •

edited

Loading

costin commented Apr 6, 2020 •

edited

Loading