SQL: change the way unsupported data types fields are handled #50823

astefan · 2020-01-09T22:18:12Z

By default, new field types in SQL are treated as UNSUPPORTED, but because "flattened" adds a _keyed sub-field to the flattened field, the fields discovery algorithm in SQL (looking at the output of field_caps API) is broken by the _keyed suffix.

This PR fixes this situation for the "flattened" data type, but also for other future unsupported data types, by marking an entire hierarchy of fields and sub-fields under an unsupported data type field, as unsupported.

as of UNSUPPORTED type

…o ignore_flattened_fields

elasticmachine · 2020-01-09T22:18:15Z

Pinging @elastic/es-search (:Search/SQL)

costin

I'm wondering if there isn't a generic approach that we can take to treat flattened as unsupported without being aware of it.
That is take into account a multi field starting with _ and potentially looking at the type and notice that we don't recognize it - essentially extending the type discovery to multi fields with special "internal" keys.

costin · 2020-01-10T09:33:30Z

x-pack/plugin/sql/build.gradle

@@ -39,6 +39,7 @@ check.dependsOn internalClusterTest

 dependencies {
  compileOnly project(path: xpackModule('core'), configuration: 'default')
+  compileOnly project(path: xpackModule('mapper-flattened'))


Unfortunately this creates a runtime issue as plugins that are not marked as extensible (such as Painless) cannot have their classes imported into a different project.
In other words the FlatObjectFieldMapper.CONTENT_TYPE won't be available which would result in a CNFE.
As such as I would simply declare the String constant internally and use it verbatim instead of linking to it.

costin · 2020-01-10T09:35:13Z

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

@@ -493,6 +495,12 @@ public void resolveAsSeparateMappings(String indexWildcard, String javaRegex, bo
                if (typeEntry.getKey().startsWith("_") && typeCap.getType().startsWith("_")) {
                    continue;
                }
+
+                // skip the "flattened" type of field's "_keyed" subfield


I wonder if we cannot make this a bit generic by looking for field names that have leaves starting with _ and if so, look at their type - if its not recognized, mark the field as unsupported.

…o ignore_flattened_fields

astefan · 2020-01-13T20:41:04Z

@elasticmachine run elasticsearch-ci/bwc

astefan · 2020-01-13T20:41:25Z

@elasticmachine run elasticsearch-ci/default-distro

astefan · 2020-01-13T22:02:46Z

@elasticmachine update branch

bpintea

LGTM

matriv

LGTM

matriv · 2020-01-14T17:04:17Z

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

@@ -495,6 +501,7 @@ public void resolveAsSeparateMappings(String indexWildcard, String javaRegex, bo
                if (typeEntry.getKey().startsWith("_") && typeCap.getType().startsWith("_")) {
                    continue;
                }
+


minor: extra new line

…o ignore_flattened_fields

of fields to improve error reporting

…o ignore_flattened_fields

costin

LGTM - added just a few comments regarding styling.

costin · 2020-01-16T10:37:54Z

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

+            String inherited = unsupportedParent.getInherited();
+            String type = unsupportedParent.getOriginalType();
+
+            if (inherited == null) {


The whole block can be further simplified by

esField = new UnsupportedEsField(esField.getName(), type, inherited != null ? inherited : unsupportedParent.getName(), esField.getProperties());

costin · 2020-01-16T10:40:03Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/Types.java

+            for (Entry<String, EsField> entry : properties.entrySet()) {
+                EsField field = entry.getValue();
+                UnsupportedEsField u;
+                if (field instanceof UnsupportedEsField) {


Why is the if needed - the result will be the same regardless of whether field is UnsupportedEsField or not.

costin · 2020-01-16T10:44:19Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

@@ -217,8 +217,14 @@ private static Attribute handleSpecialFields(UnresolvedAttribute u, Attribute na
            // unsupported types
            else if (DataTypes.isUnsupported(fa.dataType())) {
                UnsupportedEsField unsupportedField = (UnsupportedEsField) fa.field();
-                named = u.withUnresolvedMessage(
-                        "Cannot use field [" + fa.name() + "] type [" + unsupportedField.getOriginalType() + "] as is unsupported");
+                if (unsupportedField.hasInherited()) {


Since the message is the same in the branch, simply do the if on the prefix

String inheritedMessage = ""; if (unsupportedField.hasInherited()) { inheritedMessage = format("in hierarchy (field [{}"], unsupportedField.getInherited())); } named = u.withUnresolvedMessage(... + inheritedMessage);

Further more replace the string concatenation in the message with MessageFormat.format to trim it a bit.

costin

Actually, I'm unclear on why propagate unsupported is needed...

costin · 2020-01-16T10:47:49Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/Types.java

@@ -91,7 +91,8 @@ private static void walkMapping(String name, Object value, Map<String, EsField>
                    break;
                case UNSUPPORTED:
                    String type = content.get("type").toString();
-                    field = new UnsupportedEsField(name, type);
+                    field = new UnsupportedEsField(name, type, null, properties);
+                    propagateUnsupportedType(name, type, properties);


Why is propagated needed? The parents should always be discovered before children which means an unsupported parent will discovered before its children, which in turn, will already know their parent is unsupported and thus become unsupported as well.

@costin If I remember well, the method pair startWalking-walkMapping does the discovery the other way around: start with a mapping looking like parent=[child=[another_child=[type:keyword]], type:text], type:foobar]], get the parent, look at properties like "it's nested", or "it's object", or "has sub-fields" here, if yes, dig into those and so on and so forth.

Then after discovering all the children, create the parent itself here. So, by the time the algorithm finds out that a root field is unsupported, the children already have some types (their own).

This approach is used only in the case of some tests that use a .json file with mappings. I made this change because I wanted to also test the error message (here) that gets printed out from the Analyzer.

matriv

LGTM

…c#50823) The hierarchy of fields/sub-fields under a field that is of an unsupported data type will be marked as unsupported as well. Until this change, the behavior was to set the unsupported data type field's hierarchy as empty. Example, considering the following hierarchy of fields/sub-fields a -> b -> c -> d, if b would be of type "foo", then b, c and d will be marked as unsupported. (cherry picked from commit 7adb286)

#51220) The hierarchy of fields/sub-fields under a field that is of an unsupported data type will be marked as unsupported as well. Until this change, the behavior was to set the unsupported data type field's hierarchy as empty. Example, considering the following hierarchy of fields/sub-fields a -> b -> c -> d, if b would be of type "foo", then b, c and d will be marked as unsupported. (cherry picked from commit 7adb286)

#51220) The hierarchy of fields/sub-fields under a field that is of an unsupported data type will be marked as unsupported as well. Until this change, the behavior was to set the unsupported data type field's hierarchy as empty. Example, considering the following hierarchy of fields/sub-fields a -> b -> c -> d, if b would be of type "foo", then b, c and d will be marked as unsupported. (cherry picked from commit 7adb286) (cherry picked from commit df36169)

…c#50823) (elastic#51220) The hierarchy of fields/sub-fields under a field that is of an unsupported data type will be marked as unsupported as well. Until this change, the behavior was to set the unsupported data type field's hierarchy as empty. Example, considering the following hierarchy of fields/sub-fields a -> b -> c -> d, if b would be of type "foo", then b, c and d will be marked as unsupported. (cherry picked from commit 7adb286)

The hierarchy of fields/sub-fields under a field that is of an unsupported data type will be marked as unsupported as well. Until this change, the behavior was to set the unsupported data type field's hierarchy as empty. Example, considering the following hierarchy of fields/sub-fields a -> b -> c -> d, if b would be of type "foo", then b, c and d will be marked as unsupported. (cherry picked from commit 7adb286)

…c#50823) The hierarchy of fields/sub-fields under a field that is of an unsupported data type will be marked as unsupported as well. Until this change, the behavior was to set the unsupported data type field's hierarchy as empty. Example, considering the following hierarchy of fields/subfields a -> b -> c -> d, if b would be of type "foo", then b, c and d will be marked as unsupported.

ywelsch · 2020-02-27T08:58:12Z

The backport PR seems to have been merged. I'm therefore removing the backport pending label here. Please shout if this is incorrect

astefan added 2 commits January 10, 2020 00:12

Properly ignore (for the moment) the "flattened" data type, treating it

b2b553a

as of UNSUPPORTED type

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

920e143

…o ignore_flattened_fields

astefan added >bug :Analytics/SQL SQL querying v8.0.0 v7.3.3 v7.6.0 v7.4.3 v7.5.2 labels Jan 9, 2020

astefan requested review from costin, bpintea and matriv January 9, 2020 22:18

costin reviewed Jan 10, 2020

View reviewed changes

astefan added 3 commits January 13, 2020 18:20

Make the unsupported fields handling a generic one

ba09e3e

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

007d6cd

…o ignore_flattened_fields

Small cleanup

c1809e5

astefan requested a review from costin January 13, 2020 16:42

bpintea approved these changes Jan 13, 2020

View reviewed changes

matriv approved these changes Jan 14, 2020

View reviewed changes

astefan added 2 commits January 15, 2020 14:05

Add one more test

0e51f43

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

330557d

…o ignore_flattened_fields

$@polyfractal$ polyfractal added v7.7.0 and removed v7.6.0 labels Jan 15, 2020

astefan added 2 commits January 15, 2020 17:51

Added a way to remember the first unsupported data type in a hierarchy

1d1d5da

of fields to improve error reporting

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

a9e9296

…o ignore_flattened_fields

astefan force-pushed the ignore_flattened_fields branch from 44cf204 to a9e9296 Compare January 15, 2020 15:59

astefan requested a review from bpintea January 15, 2020 16:01

bpintea approved these changes Jan 15, 2020

View reviewed changes

costin approved these changes Jan 16, 2020

View reviewed changes

costin reviewed Jan 16, 2020

View reviewed changes

matriv approved these changes Jan 17, 2020

View reviewed changes

astefan added the v7.6.1 label Jan 20, 2020

astefan merged commit 7adb286 into elastic:master Jan 20, 2020

astefan added the backport pending label Jan 20, 2020

astefan changed the title ~~SQL: Treat flattened data type fields as UNSUPPORTED~~ SQL: change the way unsupported data types fields are handled Jan 20, 2020

astefan mentioned this pull request Jan 20, 2020

SQL: change the way unsupported data types fields are handled (#50823) #51220

Merged

astefan mentioned this pull request Jan 21, 2020

SQL: change the way unsupported data types fields are handled #51251

Merged

astefan deleted the ignore_flattened_fields branch January 21, 2020 14:55

astefan removed the v7.3.3 label Jan 21, 2020

$@polyfractal$ polyfractal added v7.6.0 and removed v7.6.1 labels Feb 7, 2020

ywelsch removed the backport pending label Feb 27, 2020

astefan added v7.5.3 and removed v7.5.2 labels Mar 4, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 3) elastic/elasticsearch-net#4534

Closed

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

astefan mentioned this pull request Oct 16, 2023

QL: Preserve subfields for invalid types #100875

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: change the way unsupported data types fields are handled #50823

SQL: change the way unsupported data types fields are handled #50823

astefan commented Jan 9, 2020 •

edited

Loading

elasticmachine commented Jan 9, 2020

costin left a comment

costin Jan 10, 2020

costin Jan 10, 2020

astefan commented Jan 13, 2020

astefan commented Jan 13, 2020

astefan commented Jan 13, 2020

bpintea left a comment

matriv left a comment

matriv Jan 14, 2020

costin left a comment

costin Jan 16, 2020

costin Jan 16, 2020

costin Jan 16, 2020

costin left a comment

costin Jan 16, 2020

astefan Jan 16, 2020

astefan Jan 17, 2020

matriv left a comment

ywelsch commented Feb 27, 2020

SQL: change the way unsupported data types fields are handled #50823

SQL: change the way unsupported data types fields are handled #50823

Conversation

astefan commented Jan 9, 2020 • edited Loading

elasticmachine commented Jan 9, 2020

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Jan 13, 2020

astefan commented Jan 13, 2020

astefan commented Jan 13, 2020

bpintea left a comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin 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

costin 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

matriv left a comment

Choose a reason for hiding this comment

ywelsch commented Feb 27, 2020

astefan commented Jan 9, 2020 •

edited

Loading