EQL: Add Substring function with Python semantics by costin · Pull Request #53688 · elastic/elasticsearch

costin · 2020-03-17T18:35:20Z

Initial function added to act as a template.
Does not reuse substring from SQL due to the difference in semantics and
the accepted arguments.
Currently it is missing any integration test as, the usage of scripting,
which might require an actual integration test against a proper cluster (and
likely its own QA project).

Initial function added to act as a template. Does not reuse substring from SQL due to the difference in semantics and the accepted arguments. Currently it is missing any integration test as, the usage of scripting, requires an actual integration test against a proper cluster (and likely its own QA project).

elasticmachine · 2020-03-17T18:35:22Z

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

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

astefan

Nice step forward. Left few comments.

astefan · 2020-03-18T11:03:19Z

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

+        while (end < 0) {
+            end += length;
+        }
+        int validEndIndex = length;


I don't think you need validEndIndex, do you?

astefan · 2020-03-18T12:09:47Z

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

+        while (start < 0) {
+            start += length;
+        }
+        while (end < 0) {


Could this while be replaced with this simple math?

if (end < 0) { end = end - end/length * length + (end % length != 0 ? length : 0); }

No so simple :)

:-) instead of subtracting length each time and then check if the value is still negative, the formula above subtracts the entire amount once by dividing the end value by length (to see how many lengths "fit" in "end") and taking the whole part only (not also the decimals), multiplying by length and adding a possible remainder (defined as end % length).

astefan · 2020-03-18T12:29:10Z

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

+    private final Expression source, start, end;
+
+    public Substring(Source source, Expression src, Expression start, Expression end) {
+        super(source, end != null ? Arrays.asList(src, start, end) : Arrays.asList(src, new Literal(source, 0, DataTypes.INTEGER), start));


If there is no end, shouldn't end be treated as the end of the string itself, meaning source's length?

astefan · 2020-03-18T12:30:23Z

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

+    public Substring(Source source, Expression src, Expression start, Expression end) {
+        super(source, end != null ? Arrays.asList(src, start, end) : Arrays.asList(src, new Literal(source, 0, DataTypes.INTEGER), start));
+        this.source = src;
+        this.start = end == null ? new Literal(source, 0, DataTypes.INTEGER) : start;


I think here you mean to use start == null, no?

astefan · 2020-03-18T12:30:57Z

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

+        super(source, end != null ? Arrays.asList(src, start, end) : Arrays.asList(src, new Literal(source, 0, DataTypes.INTEGER), start));
+        this.source = src;
+        this.start = end == null ? new Literal(source, 0, DataTypes.INTEGER) : start;
+        this.end = end == null ? start : end;


If end == null shouldn't end be the length of the string itself?

astefan · 2020-03-18T12:39:25Z

...rg/elasticsearch/xpack/eql/expression/function/scalar/string/SubstringFunctionProcessor.java

+        if (start == null || end == null) {
+            return source;
+        }
+        if (!(start instanceof Number)) {


== false instead of !?

astefan · 2020-03-18T12:39:51Z

...rg/elasticsearch/xpack/eql/expression/function/scalar/string/SubstringFunctionProcessor.java

+        if (!(end instanceof Number)) {
+            throw new EqlIllegalArgumentException("A number is required; received [{}]", end);
+        }
+        if (((Number) end).intValue() < 0) {


I thought end can be negative?

astefan · 2020-03-18T12:40:36Z

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

+
+    InternalEqlScriptUtils() {}
+
+


Extra empty line.

astefan · 2020-03-18T12:42:24Z

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/analysis/VerifierTests.java

        assertEquals("1:15: Unknown function [add]",
                error("process where add(serial_event_id, 0) == 1"));
-        assertEquals("1:15: Unknown function [subtract]",
+        assertEquals("1:15: Unknown function [subtract], did you mean [substring]?",


astefan · 2020-03-18T12:48:34Z

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

+
+import static org.elasticsearch.xpack.eql.expression.function.scalar.string.StringUtils.substringSlice;
+
+public class StringUtilsTests extends ESTestCase {


No tests with null values?

costin · 2020-03-18T14:49:18Z

I've updated the algorithm since my understanding of sequence based on the docs was not correct; so I experimented with the Python REPL and came up with the above.

matriv

Left also some comments mostly minor.

matriv · 2020-03-18T13:02:13Z

...a/org/elasticsearch/xpack/ql/expression/function/scalar/whitelist/InternalQlScriptUtils.java

 import java.util.Map;

-public abstract class InternalQlScriptUtils {
+public class InternalQlScriptUtils {


could be also final ?

No because it is extended by the other Script classes.

ah yes, sorry.

matriv · 2020-03-18T13:03:20Z

...ck/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/action/AbstractEqlIntegTestCase.java

    @Override
    protected Collection<Class<? extends Plugin>> nodePlugins() {
-        return Collections.singletonList(LocalStateEqlXPackPlugin.class);
+        return Arrays.asList(LocalStateEqlXPackPlugin.class);


Why is that?

matriv · 2020-03-18T21:53:40Z

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

+        if (end < 0) {
+            end = 0;
+        }
+        if (end > length) {


This check couldn't be included inside the

if (end < 0)

?

No because once checks the lower limit, whether end is still negative (after potentially adding length) while the other checks the upper limit.

matriv · 2020-03-18T21:55:33Z

...ava/org/elasticsearch/xpack/eql/expression/function/scalar/string/SubstringFunctionPipe.java

+        return start;
+    }
+
+    public Pipe length() {


Here also, shouldn't be end?

matriv · 2020-03-18T22:01:30Z

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

+        assertEquals(str.substring(5, 9), substringSlice(str, -5, -1));
+    }
+
+    public void testSubstringSliceNegativeOverLength() {


How about both negative and both higher than length?
Also a test for both positive and both higher than end.

That's what the test does - -15 and -11 are both negative and higher than length.
Am I missing something?

Sorry my bad about the negative.

matriv · 2020-03-18T22:02:49Z

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

+        assertEquals(str.substring(0, 10 - 1), substringSlice(str, -start, -1));
+    }
+
+    public void testEndHigherThanLenght() {


Suggested change

public void testEndHigherThanLenght() {

public void testEndHigherThanLength() {

astefan

LGTM. As a side note (and a separate PR), I think we need to add NodeSubclassTests to EQL, similar to QL's and SQL's corresponding classes.

matriv · 2020-03-19T14:10:05Z

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

+    }
+
+    public void testNullValue() {
+        assertNull(substringSlice(null, 0, 0));


Maybe random ints (negative too) for completeness?

Those are covered in testSubstringRandomSliceNegative

Improve separation of scripting between EQL and SQL by delegating common methods to QL. The context detection is determined based on the package to avoid having repetitive class hierarchies. Relates elastic#53688

Improve separation of scripting between EQL and SQL by delegating common methods to QL. The context detection is determined based on the package to avoid having repetitive class hierarchies. The Painless whitelists have been improved so that the declaring class is used instead of the inherited one. Relates #53688

Improve separation of scripting between EQL and SQL by delegating common methods to QL. The context detection is determined based on the package to avoid having repetitive class hierarchies. The Painless whitelists have been improved so that the declaring class is used instead of the inherited one. Relates #53688 (cherry picked from commit 6d46033) EQL: Add Substring function with Python semantics (#53688) Does not reuse substring from SQL due to the difference in semantics and the accepted arguments. Currently it is missing full integration tests as, due to the usage of scripting, requires an actual integration test against a proper cluster (and likely its own QA project). (cherry picked from commit f58680b)

costin added the :Analytics/EQL EQL querying label Mar 17, 2020

costin requested review from aleksmaus, astefan, matriv and rw-access March 17, 2020 18:35

rw-access reviewed Mar 17, 2020

View reviewed changes

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

rw-access reviewed Mar 17, 2020

View reviewed changes

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

rw-access reviewed Mar 17, 2020

View reviewed changes

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

astefan reviewed Mar 18, 2020

View reviewed changes

costin added 2 commits March 18, 2020 16:06

Merge branch 'master' into eql/functions-nointeg

8499f3c

Address feedback

537d870

Fix incorrect assignment

7db46a6

costin mentioned this pull request Mar 18, 2020

EQL: Add support for existing functions #51556

Closed

18 tasks

matriv reviewed Mar 18, 2020

View reviewed changes

costin added 3 commits March 19, 2020 14:10

Address feedback

ffd50c8

Fix super initialization

bd49db7

Another rename

85eec10

astefan self-requested a review March 19, 2020 12:39

Fix failing test

b7c8ba9

astefan approved these changes Mar 19, 2020

View reviewed changes

matriv reviewed Mar 19, 2020

View reviewed changes

costin merged commit f58680b into elastic:master Mar 19, 2020

costin deleted the eql/functions-nointeg branch March 19, 2020 14:58

costin mentioned this pull request Mar 22, 2020

EQL: Fix custom scripting for functions #53935

Merged

costin mentioned this pull request Mar 24, 2020

EQL: Fix custom scripting for functions (#53935) #54114

Merged


		import static org.elasticsearch.xpack.eql.expression.function.scalar.string.StringUtils.substringSlice;

		public class StringUtilsTests extends ESTestCase {

	public void testEndHigherThanLenght() {
	public void testEndHigherThanLength() {

Conversation

costin commented Mar 17, 2020

Uh oh!

elasticmachine commented Mar 17, 2020

Uh oh!

Uh oh!

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!

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!

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

Uh oh!

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

matriv Mar 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv Mar 18, 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!

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!

Reviewers

Assignees

Labels

Projects

costin commented Mar 18, 2020 •

edited

Loading

matriv Mar 18, 2020 •

edited

Loading

matriv Mar 18, 2020 •

edited

Loading