Integrate "fields" API into QL by astefan · Pull Request #68467 · elastic/elasticsearch

astefan · 2021-02-03T16:27:27Z

This is a first step towards switching from using docvalue_fields and _source extraction to using the fields API. It helps simplify the QL code, but does also come with a small drawback which will become visible in FieldHitExtractorTests where some tests are not needed anymore. Any query (especially SQL) on fields that have _source disabled, but are indexed (accessible through docvalue_fields), will not be possible anymore. The tests in the above mentioned class are changed to adapt to this new restriction by checking that an exception is being thrown in those cases.

Future PR(s) against this branch will probably simplify the code even further.
Also, there will be another PR that will add BWC tests similar to this one.

Relates to #67727.

elasticmachine · 2021-02-03T16:27:34Z

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

astefan · 2021-02-03T21:02:18Z

@elasticmachine run elasticsearch-ci/default-distro

costin

LGTM. Glad to see this PR land. The -700 lines is just icing on the cake.
Curious about the impact it will have in benchmarks once it lands.

I would mark it as breaking change even if only to indicate the response of the translate endpoint changes.

costin · 2021-02-04T08:30:12Z

.../src/main/java/org/elasticsearch/xpack/eql/execution/search/extractor/FieldHitExtractor.java

    }

-    public FieldHitExtractor(String name, String fullFieldName, DataType dataType, ZoneId zoneId, boolean useDocValue, String hitName,
+    public FieldHitExtractor(String name, DataType dataType, ZoneId zoneId, String hitName,


costin · 2021-02-04T08:31:23Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/execution/search/QlSourceBuilder.java

        scriptFields.forEach(sourceBuilder::scriptField);
    }

    public boolean noSource() {


Likely this needs to go in a future PR since the notion of source is removed.

Yes, I also believe there is room for improvement when it comes to removing code. There may be other places where things are not needed anymore.

(seems to only be used once?)

costin · 2021-02-04T08:33:11Z

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


     city:s    |     location:s
-Amsterdam      |POINT (4.850311987102032 52.347556999884546)
+Amsterdam      |POINT (4.850312 52.347557)


Any idea where does the rounding come from?

@costin "fields" API is relying (atm) on extraction from _source, which is basically the input the user used when indexing. Before "fields" API we were relying mainly on extraction from _source but there were field types (geo_point and shape being two of them) that we were taking from docvalues (meaning the actual values that were indexed). And in docvalues, things are just a bit different when it comes to floating point numbers and geo stuff.

That specific document with Amsterdam city has been indexed as "city": "Amsterdam", "location": "52.347557,4.850312" (location is a geo_point) and the returned value is pretty much what has been actually sent as _source to ES.

costin · 2021-02-04T08:34:30Z

...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java

@@ -187,8 +187,5 @@ private static void optimize(QueryContainer query, SearchSourceBuilder builder)

    private static void disableSource(SearchSourceBuilder builder) {
        builder.fetchSource(FetchSourceContext.DO_NOT_FETCH_SOURCE);


Is there any downside to disabling stored fields?

I hope not @costin . Tests passed. I don't see why we would want to return stored fields... can't think of a reason why we had it there in the first place. In theory, we shouldn't need any kind of way of returning values from Elasticsearch except "fields".

The why not disable explicitly stored fields:

builder.storedFields(NO_STORED_FIELD);

Ok. Will try that.

@costin I forgot about this part. "fields" API does not allow disabling stored fields.

matriv

LGTM, left a comment for the unit test.

matriv · 2021-02-04T09:48:05Z

...lugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/FieldExtractorTestCase.java

-        expected.put("columns", Arrays.asList(columnInfo("plain", "keyword_field", "keyword", JDBCType.VARCHAR, Integer.MAX_VALUE)));
-        expected.put("rows", singletonList(singletonList(ignoreAbove ? null : keyword)));
-        assertResponse(expected, runSql("SELECT keyword_field FROM test"));
+        if (explicitSourceSetting == false || enableSource) {


Maybe you can extract a method, where you pass the expected Map and the query.

Makes sense. Will look into simplifying the code here.

bpintea

Nice work, LGTM.

bpintea · 2021-02-04T11:26:39Z

...in/java/org/elasticsearch/xpack/ql/execution/search/extractor/AbstractFieldHitExtractor.java

    }

-    protected Object unwrapMultiValue(Object values) {
+    protected Object unwrapFieldsMultiValue(Object values) {


Why the "Fields" plural?

Good question. If I remember well, it's a leftover from a previous attempt where both strategies were used at the same time in code: extraction from source and docvalues (unwrapSourceMultiValue) and extraction from "fields" API output (unwrapFieldsMultiValue). No need for the rename, indeed. I'll revert.

bpintea · 2021-02-04T11:33:33Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/execution/search/QlSourceBuilder.java

        scriptFields.forEach(sourceBuilder::scriptField);
    }

    public boolean noSource() {


(seems to only be used once?)

bpintea · 2021-02-04T11:42:18Z

...lugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/FieldExtractorTestCase.java

     *    }
     */
    public void testKeywordField() throws IOException {
+        String query = "SELECT keyword_field FROM test";


(nice, prepares the reader for what to expect.)

@bpintea regarding noSource(), agree, it's used once. As part of @costin's review I already removed noSource() but didn't commit the change just yet.

* Integrate "fields" API into QL (#68467) * QL: retry SQL and EQL requests in a mixed-node (rolling upgrade) cluster (#68602) * Adapt nested fields extraction from "fields" API output to the new un-flattened structure (#68745)

* Integrate "fields" API into QL (elastic#68467) * QL: retry SQL and EQL requests in a mixed-node (rolling upgrade) cluster (elastic#68602) * Adapt nested fields extraction from "fields" API output to the new un-flattened structure (elastic#68745) (cherry picked from commit ee5cc54)

* Integrate "fields" API into QL (#68467) * QL: retry SQL and EQL requests in a mixed-node (rolling upgrade) cluster (#68602) * Adapt nested fields extraction from "fields" API output to the new un-flattened structure (#68745) (cherry picked from commit ee5cc54)

Integrate "fields" API into QL

72b1135

astefan added >enhancement :Analytics/SQL SQL querying :Analytics/EQL EQL querying labels Feb 3, 2021

astefan requested review from bpintea, costin, matriv and palesz February 3, 2021 16:27

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

Fix yml test

8960cb4

costin approved these changes Feb 4, 2021

View reviewed changes

matriv approved these changes Feb 4, 2021

View reviewed changes

bpintea approved these changes Feb 4, 2021

View reviewed changes

Address review

af7934c

astefan merged commit 174fa45 into elastic:ql_fields_api_implementation Feb 4, 2021

astefan deleted the fields_api_part1 branch February 4, 2021 22:12

astefan added a commit to astefan/elasticsearch that referenced this pull request Feb 5, 2021

Integrate "fields" API into QL (elastic#68467)

c70de0c

bpintea mentioned this pull request Feb 8, 2021

SQL: add multivalue field support (arrays) #68066

Closed

6 tasks

astefan mentioned this pull request Feb 10, 2021

QL: "fields" api implementation in QL #68802

Merged

		@@ -187,8 +187,5 @@ private static void optimize(QueryContainer query, SearchSourceBuilder builder)

		private static void disableSource(SearchSourceBuilder builder) {
		builder.fetchSource(FetchSourceContext.DO_NOT_FETCH_SOURCE);

Conversation

astefan commented Feb 3, 2021

Uh oh!

elasticmachine commented Feb 3, 2021

Uh oh!

astefan commented Feb 3, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments