EQL: implement math functions: add, divide, module, multiply, subtract by aleksmaus · Pull Request #55137 · elastic/elasticsearch

aleksmaus · 2020-04-14T00:20:52Z

Related to #54846

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

...in/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Arithmetics.java

aleksmaus · 2020-04-14T00:34:23Z

and of course QL tests fail with

20:23:05 org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryArithmeticProcessorTests > testTree FAILED
20:23:05     java.lang.AssertionError: expected:<1> but was:<1.0>
20:23:05         at __randomizedtesting.SeedInfo.seed([1FE6DDAE374DA7C1:8FF2CE9796CC0ED1]:0)

and

20:23:06 org.elasticsearch.xpack.ql.optimizer.OptimizerRulesTests > testArithmeticFolding FAILED
20:23:06     java.lang.AssertionError: expected:<2> but was:<2.3333333333333335>

Will reach out tomorrow.

elasticmachine · 2020-04-15T09:11:51Z

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

matriv

LGTM

imotov

The division by 0 exception currently look like this:

{
  "error" : {
    "root_cause" : [
      {
        "type" : "script_exception",
        "reason" : "runtime error",
        "script_stack" : [
          "org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Arithmetics.div(Arithmetics.java:100)",
          "org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Arithmetics$NumericArithmetic.wrap(Arithmetics.java:30)",
          "org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.doApply(DefaultBinaryArithmeticOperation.java:45)",
          "org.elasticsearch.xpack.ql.expression.predicate.PredicateBiFunction.apply(PredicateBiFunction.java:24)",
          "org.elasticsearch.xpack.ql.expression.function.scalar.whitelist.InternalQlScriptUtils.div(InternalQlScriptUtils.java:128)",
          "InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq(InternalQlScriptUtils.div(params.v0,InternalQlScriptUtils.docValue(doc,params.v1)),params.v2))",
          "                                                                                                                       ^---- HERE"
        ],
        "script" : "InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq(InternalQlScriptUtils.div(params.v0,InternalQlScriptUtils.docValue(doc,params.v1)),params.v2))",
        "lang" : "painless",
        "position" : {
          "offset" : 119,
          "start" : 0,
          "end" : 156
        }
      }
    ],
    "type" : "search_phase_execution_exception",
    "reason" : "all shards failed",
    "phase" : "query",
    "grouped" : true,
    "failed_shards" : [
      {
        "shard" : 0,
        "index" : "endgame",
        "node" : "hOOT6db-REOOt2zKF-eYRQ",
        "reason" : {
          "type" : "script_exception",
          "reason" : "runtime error",
          "script_stack" : [
            "org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Arithmetics.div(Arithmetics.java:100)",
            "org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Arithmetics$NumericArithmetic.wrap(Arithmetics.java:30)",
            "org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.doApply(DefaultBinaryArithmeticOperation.java:45)",
            "org.elasticsearch.xpack.ql.expression.predicate.PredicateBiFunction.apply(PredicateBiFunction.java:24)",
            "org.elasticsearch.xpack.ql.expression.function.scalar.whitelist.InternalQlScriptUtils.div(InternalQlScriptUtils.java:128)",
            "InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq(InternalQlScriptUtils.div(params.v0,InternalQlScriptUtils.docValue(doc,params.v1)),params.v2))",
            "                                                                                                                       ^---- HERE"
          ],
          "script" : "InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq(InternalQlScriptUtils.div(params.v0,InternalQlScriptUtils.docValue(doc,params.v1)),params.v2))",
          "lang" : "painless",
          "position" : {
            "offset" : 119,
            "start" : 0,
            "end" : 156
          },
          "caused_by" : {
            "type" : "arithmetic_exception",
            "reason" : "/ by zero"
          }
        }
      }
    ]
  },
  "status" : 400
}

I wonder if it might make sense to suppress it the same way we do in SQL, where it looks like this:

{
  "error" : {
    "root_cause" : [
      {
        "type" : "arithmetic_exception",
        "reason" : "/ by zero"
      }
    ],
    "type" : "arithmetic_exception",
    "reason" : "/ by zero"
  },
  "status" : 500
}

astefan

Left few comments.

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

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

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

astefan · 2020-04-21T11:01:46Z

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

-
-[[queries]]
-expected_event_ids = [82]
-query = "file where serial_event_id % 40 == 2"


Can we have more complex math operations?
Few ideas:

put the constant first and the field name second

use the minus sign for the field as well: file where -serial_event_id + 100 == 18

have a more complex arithmetic set of operations: file where serial_event_id + ((1 + 3) * 2 / (3 - 1)) * 2 == 90 OR 70 + serial_event_id < 100

Added all of the above.

x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt

aleksmaus · 2020-04-23T03:51:27Z

@imotov I like the idea for simpler response for this kind of cases. I compared with the SQL execution code, it doesn't look like it uses "painless" though, so the exception from div raised pretty much to the response. Need to dig to understand better. Can look into this after merging this PR.

astefan

In general LGTM. But I left few minor comments.

astefan · 2020-04-23T08:35:31Z

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

 expected_event_ids = [82, 83]
 query = "file where serial_event_id / 2 == 41"
+
+# Additional EQL queries with arithmetic operations that were not part of the original EQL implementation


It would be worth adding these new tests to the original EQL implementation, as well.

astefan · 2020-04-23T08:38:17Z

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

+
+[[queries]]
+expected_event_ids = [82]
+query = "file where serial_event_id + ((1 + 3) * 2 / (3 - 1)) * 2 == 90 or 70 + serial_event_id < 100"


I don't think this is correct.
70 + serial_event_id < 100 should match all event IDs from 1 to 29. Why is expected_event_ids only 82? Or I am missing something?

You are absolutely right.
Looks like something changed, even the simple query like
doesn't return all 102 records:

1> [2020-04-23T19:17:13,335][INFO ][o.e.x.e.EqlActionIT ] [test] [65.test -> file where true] before test 1> [2020-04-23T19:17:13,346][INFO ][o.e.x.e.EqlActionIT ] [test] [55, 59, 60, 61, 65, 66, 67, 70, 71, 72, 75, 76, 77, 81, 82, 83, 86, 87, 88, 91, 92, 95, 96]

looking

nevermind .... the other events have different type

Updated to query against process type that leads to more results

astefan · 2020-04-23T08:39:14Z

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

+            },
+        // Arithmetic
+            new FunctionDefinition[] {
+                    def(Add.class, Add::new,"add"),


Whitespace before the last parameter.

rw-access · 2020-04-23T14:46:52Z

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

+
+[[queries]]
+expected_event_ids = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29]
+query = "process where serial_event_id + ((1 + 3) * 2 / (3 - 1)) * 2 == 90 or 70 + serial_event_id < 100"


This looks like it tests the query folding more than the integration, and folds to

process where serial_event_id + 8 == 90 or serial_event_id < 100.

Since serial_event_id of 82 is a file event, we don't have any results that match the left side.

Yep, thanks, updated the test to match the left side of or condition.

rw-access

left a comment about the test_queries_supported.toml, but other than that lgtm

elastic#55137) * EQL: implement math functions: add, divide, module, multiply, subtract

#55137) (#55737) * EQL: implement math functions: add, divide, module, multiply, subtract

EQL: implement math functions: add, divide, module, multiply, subtract

5e43d9e

aleksmaus requested review from costin and rw-access April 14, 2020 00:20

aleksmaus commented Apr 14, 2020

View reviewed changes

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

...in/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Arithmetics.java Outdated Show resolved Hide resolved

cbuescher added the :Analytics/EQL EQL querying label Apr 15, 2020

costin added the team-discuss label Apr 16, 2020

costin requested review from astefan and matriv April 16, 2020 11:18

Address feeback on PR draft.

f02a483

aleksmaus removed the team-discuss label Apr 17, 2020

aleksmaus requested a review from imotov April 17, 2020 01:28

aleksmaus marked this pull request as ready for review April 17, 2020 01:28

Merge branch 'master' into feature/eql_math

2a7c7fc

matriv approved these changes Apr 20, 2020

View reviewed changes

imotov reviewed Apr 20, 2020

View reviewed changes

astefan reviewed Apr 21, 2020

View reviewed changes

aleksmaus added 2 commits April 22, 2020 08:58

Merge branch 'master' into feature/eql_math

01de50c

Address code review feedback

1f1a253

astefan approved these changes Apr 23, 2020

View reviewed changes

Address the latest code review comments

c8d8122

rw-access reviewed Apr 23, 2020

View reviewed changes

rw-access approved these changes Apr 23, 2020

View reviewed changes

Update test based on the code review feedback

0f3c75a

aleksmaus merged commit 9e79e5d into elastic:master Apr 24, 2020

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

EQL: implement math functions: add, divide, module, multiply, subtract (

0f1ae6d

elastic#55137) * EQL: implement math functions: add, divide, module, multiply, subtract

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

EQL: implement math functions: add, divide, module, multiply, subtract (

ad54cca

#55137) (#55737) * EQL: implement math functions: add, divide, module, multiply, subtract

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

elasticmachine commented Apr 15, 2020

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

imotov left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aleksmaus commented Apr 23, 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!

aleksmaus Apr 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!

aleksmaus Apr 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!

Choose a reason for hiding this comment

Uh oh!

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

rw-access left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aleksmaus commented Apr 14, 2020 •

edited

Loading

aleksmaus commented Apr 14, 2020 •

edited

Loading

aleksmaus Apr 23, 2020 •

edited

Loading

aleksmaus Apr 23, 2020 •

edited

Loading

rw-access Apr 23, 2020 •

edited

Loading