EQL: Add number function by rw-access · Pull Request #55084 · elastic/elasticsearch

rw-access · 2020-04-10T19:02:19Z

Resolves #54471

Added function to parse numbers from strings.
One note is that the resolved DataType is double. Since it can return integer, float, or number. Double seemed to be the most inclusive.

elasticmachine · 2020-04-10T19:02:21Z

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

astefan

Left a bunch of comments.
I think we need more integration and unit tests to cover all scenarios (see comment for the toml file).
Also, base argument has been left off in quite few places.

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

astefan · 2020-04-15T09:16:35Z

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

+
+    @Override
+    public final Pipe resolveAttributes(AttributeResolver resolver) {
+        Pipe newSource = source.resolveAttributes(resolver);


Did you forget the base here?

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

astefan · 2020-04-15T09:19:07Z

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

                def(EndsWith.class, EndsWith::new, "endswith"),
                def(IndexOf.class, IndexOf::new, "indexof"),
                def(Length.class, Length::new, "length"),
+                def(ToNumber.class, ToNumber::new, "number"),


I think we should add a new FunctionDefinition[] array for math/numerical functions.

agreed. @aleksmaus has one in his PR. I wasn't sure if this was math or still string, since it's parsing from strings

astefan · 2020-04-15T09:32:12Z

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

+        try {
+            return NumberFormat.getNumberInstance(Locale.US).parse(source.toString());
+        } catch (ParseException e) {
+            throw new EqlIllegalArgumentException("Unable to convert [{}] to number of base [{}]", source, radix);


Here I would prefer to see in the error message the original base. If that is null because the user didn't provide one, then print radix.

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

astefan · 2020-04-15T09:48:22Z

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

 expected_event_ids  = [1, 2, 3, 5, 11]
 description = "test built-in math functions"

-[[queries]]


Can you add more tests here, please?
The way I see it, there are tests (just one) for no argument number function and for base 16. How about base 2, explicit base 10, and custom bases?

I would still like to see here more tests.

Added tests for the processor, and added integration tests here
867fc01

astefan

Looking better.
Added a second round of comments.

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

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

astefan · 2020-04-22T14:32:59Z

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

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


I believe we should add a new package for the numeric/math functions. At the moment this is ...function.scalar.string. ES SQL has .math. Probably worth doing the same here. Add a package ...function.scalar.math.

astefan · 2020-04-22T14:33:31Z

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

 expected_event_ids  = [1, 2, 3, 5, 11]
 description = "test built-in math functions"

-[[queries]]


I would still like to see here more tests.

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

...va/org/elasticsearch/xpack/eql/expression/function/scalar/string/ToNumberProcessorTests.java

astefan

LGTM. Left two very minor comments though.

.../eql/src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/math/ToNumber.java

astefan · 2020-05-13T12:10:27Z

...java/org/elasticsearch/xpack/eql/expression/function/scalar/math/ToNumberProcessorTests.java

+        assertNull(process(null, null));
+        assertNull(process(null, 8));
+        assertNull(process(null, 10));
+        assertNull(process(null, 16));


assertNull(process(123, null)) is missing.

ah. that's because if the base is null, then the base will be autodetected.
I don't know if there's a way to mention that the parameter was not supplied vs it was evaluated as false. I believe substring() and a few other functions have similar behavior

aleksmaus

Left one question, otherwise LGTM.
Thank you!

...a/org/elasticsearch/xpack/eql/expression/function/scalar/math/ToNumberFunctionProcessor.java

* EQL: Add number function * EQL: Fix the locale used for number for deterministic functionality * EQL: Add more ToNumber tests * EQL: Add more number ToNumberProcessor unit tests * EQL: Remove unnecessary overrides, fix processor methods * EQL: Remove additional unnecessary overrides * EQL: Lint fixes for ToNumber * EQL: ToNumber renames from PR feedback * EQL: Remove NumberFormat locale handling * EQL: Removed NumberFormat from ToNumber * EQL: Add number function tests * EQL: ToNumberProcessorTests formatting * EQL: Remove newline in ToNumberProcessorTests * EQL: Add number(..., null) test * EQL: Create expression.function.scalar.math package * EQL: Remove painless whitespace for ToNumber.asScript * EQL: Add Long support

rw-access added 2 commits April 10, 2020 12:52

EQL: Add number function

8309b8f

EQL: Fix the locale used for number for deterministic functionality

4b698eb

rw-access added the :Analytics/EQL EQL querying label Apr 10, 2020

rw-access requested review from aleksmaus, astefan and costin April 10, 2020 19:02

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

750f6bb

astefan requested changes Apr 15, 2020

View reviewed changes

rw-access added 10 commits April 16, 2020 09:56

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

82e8076

EQL: Add more ToNumber tests

ea3d949

EQL: Add more number ToNumberProcessor unit tests

20aa77f

EQL: Remove unnecessary overrides, fix processor methods

867e122

EQL: Remove additional unnecessary overrides

e2321d8

EQL: Lint fixes for ToNumber

a123c7d

EQL: ToNumber renames from PR feedback

86c9949

EQL: Remove NumberFormat locale handling

c1dac67

EQL: Removed NumberFormat from ToNumber

22ec36d

Merge branch 'master' into eql/number-function

0e8d77c

rw-access requested a review from astefan April 21, 2020 20:34

astefan reviewed Apr 22, 2020

View reviewed changes

rw-access added 4 commits April 22, 2020 13:57

EQL: Add number function tests

5e23e9a

EQL: ToNumberProcessorTests formatting

1b0ac6f

EQL: Remove newline in ToNumberProcessorTests

867fc01

EQL: Add number(..., null) test

33e2ffa

rjernst added the Team:QL (Deprecated) Meta label for query languages team label May 4, 2020

rw-access added 4 commits May 5, 2020 14:06

Merge branch 'master' into eql/number-function

b49ea23

Create expression.function.scalar.math package

a6501b4

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

1ae345c

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

326328d

rw-access requested a review from astefan May 8, 2020 17:36

astefan approved these changes May 13, 2020

View reviewed changes

rw-access added 2 commits May 13, 2020 08:36

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

faa3fe5

EQL: Remove painless whitespace for ToNumber.asScript

2ef2ce4

aleksmaus approved these changes May 13, 2020

View reviewed changes

...a/org/elasticsearch/xpack/eql/expression/function/scalar/math/ToNumberFunctionProcessor.java Outdated Show resolved Hide resolved

...a/org/elasticsearch/xpack/eql/expression/function/scalar/math/ToNumberFunctionProcessor.java Outdated Show resolved Hide resolved

EQL: Add Long support

c2e9023

rw-access merged commit b0863cc into elastic:master May 13, 2020

rw-access deleted the eql/number-function branch May 13, 2020 20:07

jrodewig mentioned this pull request May 14, 2020

[DOCS] EQL: Document number function #56770

Merged

Conversation

rw-access commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 10, 2020

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rw-access Apr 22, 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!

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

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aleksmaus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rw-access commented Apr 10, 2020 •

edited

Loading

rw-access Apr 22, 2020 •

edited

Loading