Adds `runtime_mappings` to EQL and SQL requests #71356

astefan · 2021-04-06T15:53:58Z

The runtime_mappings request element will allow EQL and SQL users to define search time runtime fields which can later be used in queries.
Addresses #68116.

define search time runtime fields which will be used in queries

elasticmachine · 2021-04-06T16:48:39Z

Pinging @elastic/es-ql (Team:QL)

costin

LGTM

The only minor observations that I have are:

use static imports for emptyList/emptyMap - same number of line changes but improves readability
if my understanding is correct, all integration tests moving forward will run with the new runtime field enable; maybe it wouldn't be too much effort to randomize this (though a separate PR). in ES this happens dynamically, it's H2 that we have to worry about (either by having two separate load files or ideally, manipulating the loaded data at runtime, similar to what ES does).

costin · 2021-04-06T18:43:09Z

x-pack/plugin/sql/qa/server/src/main/resources/employees.csv

-1961-09-23T00:00:00Z,10098,Sreekrishna,F,1985-05-13T00:00:00Z,4,Servieres,44817
-1956-05-25T00:00:00Z,10099,Valter,F,1988-10-18T00:00:00Z,2,Sullins,73578
-1953-04-21T00:00:00Z,10100,Hironobu,F,1987-09-21T00:00:00Z,4,Haraldson,68431
+birth_date,birth_date_day_of_week,emp_no,first_name,gender,hire_date,languages,last_name,salary


The extra column can be generated at runtime in H2 through the SELECT of CSVREAD function - instead of doing just SELECT * do a SELECT *, DAY_OF_THE_WEEK(birth_date)orSELECT birth_date, DAY_OF_THE_WEEK(birth_date) AS birth_date_dow, ...`
If that doesn't work do the insert as is followed by an update inside the sql script.

That's a good idea I haven't thought of. I'll change the way data is loaded in H2.

bpintea

LGTM.
I take the docs would be updated subsequently?

bpintea · 2021-04-07T08:31:02Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlSearchRequest.java

+            String fieldName = entry.getKey();
+            if (entry.getValue() instanceof Map) {
+                @SuppressWarnings("unchecked")
+                Map<String, Object> propNode = new HashMap<>(((Map<String, Object>) entry.getValue()));


why is a new map instance necessary?
(and same in AbstractSqlQueryRequest.java)

bpintea · 2021-04-07T09:08:12Z

x-pack/plugin/sql/qa/server/src/main/resources/conditionals.csv-spec

 ;
+
+ifNullWithRuntimeField
+SELECT COUNT(*) c, IFNULL(birth_date_day_of_week, 'Some day') AS day FROM test_emp GROUP BY day ORDER BY c;


'no day'? :-)

matriv

Great stuff @astefan! Very nice testing!
Left some mostly minor comments.

matriv · 2021-04-07T10:29:04Z

...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumns.java

        // special case for '%' (translated to *)
        if ("*".equals(idx)) {
-            session.indexResolver().resolveAsSeparateMappings(idx, regex, includeFrozen,
+            session.indexResolver().resolveAsSeparateMappings(idx, regex, includeFrozen, Collections.emptyMap(),


nit: here and below you could use the static import.

matriv · 2021-04-07T10:30:07Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java

        }

+        // set runtime mappings
+        if (this.cfg.runtimeMappings() != null && this.cfg.runtimeMappings().isEmpty() == false) {


Why do we need this check? is it possible to be set twice or more?

matriv · 2021-04-07T10:31:29Z

...in/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java

        if (request.cursor() != null) {
            builder.field(CURSOR_NAME, request.cursor());
        }
+        if (request.runtimeMappings() != null && request.runtimeMappings().isEmpty() == false) {


Same here, why do we need to check if it's empty?
maybe an assertion that are empty could be better?

matriv · 2021-04-07T10:33:41Z

x-pack/plugin/sql/qa/server/src/main/resources/docs/docs.csv-spec

-1953-09-02T00:00:00Z|10001          |Georgi         |M              |1986-06-26T00:00:00Z|2              |Facello        |57305 
+     birth_date     |birth_date_day_of_week|    emp_no     |  first_name   |    gender     |     hire_date      |   languages   |   last_name   |    salary     
+--------------------+----------------------+---------------+---------------+---------------+--------------------+---------------+---------------+---------------
+1953-09-02T00:00:00Z|Wednesday             |10001          |Georgi         |M              |1986-06-26T00:00:00Z|2              |Facello        |57305          


nit: here and below, trailing whitespaces.

matriv · 2021-04-07T10:34:31Z

x-pack/plugin/sql/qa/server/src/main/resources/pivot.csv-spec

-null                 |10044          |Mingsen        |F              |1994-05-21 00:00:00.0|Casley         |39728          |null
-1952-04-19 00:00:00.0|10009          |Sumant         |F              |1985-02-18 00:00:00.0|Peac           |66174          |null
-1953-01-07 00:00:00.0|10067          |Claudi         |M              |1987-03-04 00:00:00.0|Stavenow       |null           |52044
+     birth_date      |birth_date_day_of_week|    emp_no     |  first_name   |    gender     |       hire_date     |   last_name   |       1       |       2       


nit: trailing whitespaces, also below.

matriv · 2021-04-07T10:40:12Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlSearchRequest.java

+            String fieldName = entry.getKey();
+            if (entry.getValue() instanceof Map) {
+                @SuppressWarnings("unchecked")
+                Map<String, Object> propNode = new HashMap<>(((Map<String, Object>) entry.getValue()));


matriv · 2021-04-07T10:40:45Z

x-pack/plugin/sql/qa/server/src/main/resources/select.csv-spec

-1959-12-03T00:00:00.000Z|10003          |Parto          |M              |1986-08-28T00:00:00.000Z|4              |Bamford        |61805
-1954-05-01T00:00:00.000Z|10004          |Chirstian      |M              |1986-12-01T00:00:00.000Z|5              |Koblick        |36174
-1955-01-21T00:00:00.000Z|10005          |Kyoichi        |M              |1989-09-12T00:00:00.000Z|1              |Maliniak       |63528
+       birth_date       |birth_date_day_of_week|    emp_no     |  first_name   |    gender     |       hire_date        |   languages   |   last_name   |    salary     


nit: trailing whitespaces.

matriv · 2021-04-07T10:41:41Z

x-pack/plugin/sql/qa/server/src/main/resources/select.csv-spec

 ;
+
+runtimeFieldWithFunctions
+SELECT CONCAT(CONCAT(SUBSTRING(birth_date_day_of_week, 0, LENGTH(birth_date_day_of_week) - 3), SPACE(11 - LENGTH(birth_date_day_of_week))), 'DAY') AS c, ISO_DAY_OF_WEEK(birth_date) AS iso FROM test_emp LIMIT 14;


Maybe an ORDER BY, to avoid non-deterministic ordering.

matriv · 2021-04-07T10:42:41Z

...sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java

+        return validationException;
+    }
+
+    private static ActionRequestValidationException validateRuntimeMappings(Map<String, Object> runtimeMappings,


Would it be possible to extract this in ql module?

Unfortunately (and frustratingly), no. sql-action has almost no dependencies on other projects. Which is a good thing for the end user, but it brings some limitations in the code reusability and organization area.

matriv · 2021-04-07T10:45:22Z

.../plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

        }, containsString("unknown field [columnar]"));
    }

+    public void testValidateRuntimeMappingsInSqlQuery() throws IOException {


If we move the validation of the RuntimeMappings in QL maybe we can have a simpler unit tests for the validation itself (incorrect json)

…o 68116_impl

matriv · 2021-04-07T14:58:30Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java


        // set runtime mappings
-        if (this.cfg.runtimeMappings() != null && this.cfg.runtimeMappings().isEmpty() == false) {
+        if (this.cfg.runtimeMappings() != null) {


Not insisting, it's a "safety net": Since we initialise the runtimeMappings with an empty map, maybe we could also remove this check.

matriv

LGTM. Thx!

* Adds `runtime_mappings` to EQL and SQL requests allowing users to define search time runtime fields which will be used in queries (cherry picked from commit b8266e5)

Adds runtime_mappings to EQL and SQL requests allowing users to

936b834

define search time runtime fields which will be used in queries

astefan added :Analytics/SQL SQL querying :Analytics/EQL EQL querying >enhancement v7.13.0 v8.0.0 labels Apr 6, 2021

astefan marked this pull request as ready for review April 6, 2021 16:48

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Apr 6, 2021

astefan requested review from bpintea, costin and matriv and removed request for matriv April 6, 2021 16:48

astefan changed the title ~~Adds runtime_mappings to EQL and SQL requests allowing users to~~ Adds runtime_mappings to EQL and SQL requests Apr 6, 2021

costin approved these changes Apr 6, 2021

View reviewed changes

costin reviewed Apr 6, 2021

View reviewed changes

bpintea approved these changes Apr 7, 2021

View reviewed changes

matriv reviewed Apr 7, 2021

View reviewed changes

astefan added 4 commits April 7, 2021 16:58

Address reviews

6c66e66

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

99d46ed

…o 68116_impl

Checkstyle fix

39c6f0b

Checkstyle fix 2

b687042

matriv reviewed Apr 7, 2021

View reviewed changes

matriv approved these changes Apr 7, 2021

View reviewed changes

astefan merged commit b8266e5 into elastic:master Apr 7, 2021

astefan deleted the 68116_impl branch April 7, 2021 16:38

astefan added the backport pending label Apr 7, 2021

stu-elastic mentioned this pull request Apr 7, 2021

[CI] JdbcSqlSpecIT testWildcardWithLimit: Different result for column [birth_date_day_of_week] #71442

Closed

jrodewig mentioned this pull request Apr 8, 2021

[DOCS] EQL/SQL: Document runtime_fields parameter #71487

Merged

astefan removed the backport pending label Apr 8, 2021

astefan mentioned this pull request Apr 8, 2021

QL: Support runtime-fields at query-time #68116

Closed

stevejgordon mentioned this pull request Apr 21, 2021

7.13.0 Meta Ticket elastic/elasticsearch-net#5584

Closed

62 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Adds runtime_mappings to EQL and SQL requests #71356

Adds runtime_mappings to EQL and SQL requests #71356

Uh oh!

Conversation

astefan commented Apr 6, 2021

Uh oh!

elasticmachine commented Apr 6, 2021

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!

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

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

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Adds `runtime_mappings` to EQL and SQL requests #71356

Adds `runtime_mappings` to EQL and SQL requests #71356