Switch from using docvalue_fields to extracting values from _source #44062

astefan · 2019-07-08T10:14:10Z

Doing this means parsing the _source and handling any fields that need to be parsed and have its outcome potentially changed by the parsers (for example, the numbers when coerce=true), just like Elasticsearch is doing it when it's indexing a document.

Fields that will not be taken from _source (but from docvalue_fields) are: keyword, date, scaled_float, geo_point, geo_shape.

This change also introduces a limitation: aliases type of fields that are NOT part of a tree of sub-fields will not be able to be retrieved anymore. field_caps API doesn't shed any light into a field being an
alias or not and at _source parsing time there is no way to know if aroot field is an alias or not. Fields of the type a.b.c.alias can be extracted from docvalue_fields, only if the field they point to can be
extracted from docvalue_fields. Also, not all fields in a hierarchy of fields can be evaluated to being an alias.

Fixes #41852.

where applicable. Doing this means parsing the _source and handling the numbers parsing just like Elasticsearch is doing it when it's indexing a document. This also introduces a minor limitation: aliases type of fields that are NOT part of a tree of sub-fields will not be able to be retrieved anymore. field_caps API doesn't shed any light into a field being an alias or not and at _source parsing time there is no way to know if a root field is an alias or not. Fields of the type "a.b.c.alias" can be extracted from docvalue_fields, only if the field they point to can be extracted from docvalue_fields. Also, not all fields in a hierarchy of fields can be evaluated to being an alias.

elasticmachine · 2019-07-08T10:14:12Z

Pinging @elastic/es-search

astefan · 2019-07-08T13:49:59Z

@elasticmachine run elasticsearch-ci/1

matriv

Nice work overall!
Left some comments but nothing major.

Also, is there a need to have the isAggregatable as an attribute of EsField class? I was thinking it can just delegate to dataType.hasDocValues(), or there is some distinction between them?

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/FieldExtractorTestCase.java

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

docs/reference/sql/limitations.asciidoc

jtibshirani · 2019-07-12T22:10:53Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/index/IndexResolver.java

@@ -520,9 +522,26 @@ public void resolveAsSeparateMappings(String indexWildcard, String javaRegex, bo
                        }
                        EsField field = indexFields.flattedMapping.get(fieldName);
                        if (field == null || (invalidField != null && (field instanceof InvalidMappedField) == false)) {
+                            int dot = fieldName.lastIndexOf('.');
+                            /*
+                             * Looking up the "tree" at the parent fields here to see if the field is an alias.


We discussed offline that this isn't a very solid way of checking whether a field is an alias. The fact that we don't return parent information about field aliases is kind of an 'accident' of the field caps logic, and I don't think it was an intentional API decision.

I filed #44298 to propose adding explicit field alias information to the field caps API, which would help make this more robust. It might also give enough information to be able to resolve + extract the alias field values from _source.

…o 41852_fix

astefan · 2019-07-18T07:02:18Z

@matriv thank you for the review. I've addressed those and added more tests and covered some more scenarios in code. Would you mind having a second look, please?

matriv

Awesome work with the extensive testing and code comments.
Left a few more comments.

matriv · 2019-07-18T10:22:22Z

docs/reference/sql/limitations.asciidoc

-`date`, `scaled_float`, `geo_point`, `geo_shape`.
+Most of {es-sql}'s columns are retrieved from the document's `_source` and there is no attempt to get the columns content from
+`docvalue_fields` not even in the case <<mapping-source-field,`_source`>> field is disabled in the mapping explicitly.
+If a column, for which there is no source stored, is asked for in a query, {es-sql} will not return it. Field types NOT returned


Maybe: Field types that don't follow this restriction are: .... since they are NOT returned from _source but from doc values.

matriv · 2019-07-18T10:28:52Z

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

@@ -84,6 +88,7 @@ public FieldHitExtractor(String name, DataType dataType, ZoneId zoneId, boolean

    FieldHitExtractor(StreamInput in) throws IOException {
        fieldName = in.readString();
+        fullFieldName = in.readOptionalString();


Don't we need some version check here to ensure backwards compatibility?

I don't think we have this in place in other place in code? I mean, at the moment we enforce strict version matching for jdbc and, I think, for cli as well....

But this is not for server-client mismatch but for version mismatch between nodes during a rolling upgrade.

matriv · 2019-07-18T10:31:09Z

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

-                            return l;
+                    Number result = null;
+                    try {
+                        result = NumberType.valueOf(dataType.esType.toUpperCase(Locale.ROOT)).parse(values, true);


Could we extract this to a method in DataType?

Which part are you referring to?

NumberType.valueOf(dataType.esType.toUpperCase(Locale.ROOT))
so it's a class method of a DataType and one can do:
dataType.numberType()

But, please, it's minor and future oriented in case we want to reuse it somewhere else.

matriv · 2019-07-18T10:34:49Z

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java

+            }
+        }
+        while (rootField.parent() != null) {
+            fullFieldName = rootField.parent().field().getName() + "." + fullFieldName;


Minor, but I think it makes sense to make fullFieldName a StringBuilder because of the potential iterations in the while loop.

matriv · 2019-07-18T14:29:51Z

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java

+        // Only if the field is not an alias (in which case it will be taken out from docvalue_fields if it's isAggregatable()),
+        // go up the tree of parents until a non-object (and non-nested) type of field is found and use that specific parent
+        // as the field to extract data from, from _source. We do it like this because sub-fields are not in the _source, only
+        // the root field to which those sub-fields belong to, are.


Would be great to add also an example with a subfield so it's more clear for someone that reads it in the future.

matriv · 2019-07-18T14:41:34Z

@astefan Should we also add a sentence in limitations page to say that if there are many docvalue fields (like date) a use can still experience the issue when doing a SELECT * ?

…o 41852_fix

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java

matriv

LGTM. Once again: great work!

…o 41852_fix

astefan · 2019-07-23T05:48:41Z

@elasticmachine run elasticsearch-ci/2

…o 41852_fix

…lastic#44062) * Switch from using docvalue_fields to extracting values from _source where applicable. Doing this means parsing the _source and handling the numbers parsing just like Elasticsearch is doing it when it's indexing a document. * This also introduces a minor limitation: aliases type of fields that are NOT part of a tree of sub-fields will not be able to be retrieved anymore. field_caps API doesn't shed any light into a field being an alias or not and at _source parsing time there is no way to know if a root field is an alias or not. Fields of the type "a.b.c.alias" can be extracted from docvalue_fields, only if the field they point to can be extracted from docvalue_fields. Also, not all fields in a hierarchy of fields can be evaluated to being an alias. (cherry picked from commit 8bf8a05)

…44062) (#44804) * Switch from using docvalue_fields to extracting values from _source where applicable. Doing this means parsing the _source and handling the numbers parsing just like Elasticsearch is doing it when it's indexing a document. * This also introduces a minor limitation: aliases type of fields that are NOT part of a tree of sub-fields will not be able to be retrieved anymore. field_caps API doesn't shed any light into a field being an alias or not and at _source parsing time there is no way to know if a root field is an alias or not. Fields of the type "a.b.c.alias" can be extracted from docvalue_fields, only if the field they point to can be extracted from docvalue_fields. Also, not all fields in a hierarchy of fields can be evaluated to being an alias. (cherry picked from commit 8bf8a05)

astefan added >enhancement :Analytics/SQL SQL querying labels Jul 8, 2019

astefan requested review from costin and matriv July 8, 2019 10:14

jtibshirani self-requested a review July 8, 2019 11:30

astefan added 2 commits July 8, 2019 14:47

Fix tests

3734353

More tests updates

36ea8be

matriv reviewed Jul 9, 2019

View reviewed changes

jtibshirani mentioned this pull request Jul 12, 2019

Consider adding field alias information to field caps. #44298

Closed

jtibshirani reviewed Jul 12, 2019

View reviewed changes

astefan added 3 commits July 17, 2019 18:15

Addressing review and adding more tests

2e5ace8

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

21c930a

…o 41852_fix

Limitations page update

a9bf6bc

astefan requested a review from matriv July 18, 2019 07:02

matriv reviewed Jul 18, 2019

View reviewed changes

astefan added 2 commits July 18, 2019 19:02

Adressing reviews

f750eaf

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

f6e0cf3

…o 41852_fix

matriv reviewed Jul 19, 2019

View reviewed changes

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java Show resolved Hide resolved

matriv approved these changes Jul 19, 2019

View reviewed changes

astefan requested review from jimczi and removed request for costin July 22, 2019 17:06

astefan added 2 commits July 22, 2019 20:26

Add backwards compatibility checks for newly added serialized info

3977431

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

142dd6b

…o 41852_fix

Small updates

f9bb0d2

astefan added 2 commits July 24, 2019 10:33

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

01cd9ec

…o 41852_fix

Fix test

c1eaf79

astefan added v7.4.0 v8.0.0 labels Jul 24, 2019

astefan merged commit 8bf8a05 into elastic:master Jul 24, 2019

astefan added the backport pending label Jul 24, 2019

astefan mentioned this pull request Jul 24, 2019

SQL: switch from using docvalue_fields to extracting values from _source 7.x backport #44804

Merged

astefan removed the backport pending label Jul 25, 2019

astefan mentioned this pull request Jul 25, 2019

[Canvas] esdocs and essql fail with more than 100 fields elastic/kibana#33070

Closed

astefan mentioned this pull request Mar 18, 2020

SQL: Use doc value only for dates, potentially keyword, use _source for everything else #38776

Closed

cbuescher mentioned this pull request Jul 22, 2020

Consolidate validation for 'docvalue_fields'. #59473

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from using docvalue_fields to extracting values from _source #44062

Switch from using docvalue_fields to extracting values from _source #44062

astefan commented Jul 8, 2019

elasticmachine commented Jul 8, 2019

astefan commented Jul 8, 2019

matriv left a comment

jtibshirani Jul 12, 2019

astefan commented Jul 18, 2019

matriv left a comment

matriv Jul 18, 2019

matriv Jul 18, 2019

astefan Jul 18, 2019

matriv Jul 19, 2019

matriv Jul 18, 2019

astefan Jul 18, 2019

matriv Jul 19, 2019

matriv Jul 19, 2019

matriv Jul 18, 2019

matriv Jul 18, 2019

matriv commented Jul 18, 2019

matriv left a comment

astefan commented Jul 23, 2019

Switch from using docvalue_fields to extracting values from _source #44062

Switch from using docvalue_fields to extracting values from _source #44062

Conversation

astefan commented Jul 8, 2019

elasticmachine commented Jul 8, 2019

astefan commented Jul 8, 2019

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Jul 18, 2019

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Jul 18, 2019

matriv left a comment

Choose a reason for hiding this comment

astefan commented Jul 23, 2019