Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jan 20, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b2b553a
Properly ignore (for the moment) the "flattened" data type, treating it
astefan Jan 9, 2020
920e143
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Jan 9, 2020
ba09e3e
Make the unsupported fields handling a generic one
astefan Jan 13, 2020
007d6cd
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Jan 13, 2020
c1809e5
Small cleanup
astefan Jan 13, 2020
0e51f43
Add one more test
astefan Jan 15, 2020
330557d
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Jan 15, 2020
1d1d5da
Added a way to remember the first unsupported data type in a hierarchy
astefan Jan 15, 2020
a9e9296
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Jan 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions x-pack/plugin/sql/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ check.dependsOn internalClusterTest

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

@costin costin Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

compileOnly(project(':modules:lang-painless')) {
exclude group: "org.ow2.asm"
}
Expand Down
8 changes: 8 additions & 0 deletions ...ck/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/index/IndexResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.index.IndexNotFoundException;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.xpack.flattened.mapper.FlatObjectFieldMapper;
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.type.DateEsField;
Expand Down Expand Up @@ -151,6 +152,7 @@ public boolean equals(Object obj) {

private static final List<String> FIELD_NAMES_BLACKLIST = Arrays.asList("_size");
private static final String UNMAPPED = "unmapped";
private static final String FLATTENED_FIELD_SUFFIX = "_keyed";

private final Client client;
private final String clusterName;
Expand Down Expand Up @@ -493,6 +495,12 @@ private static List<EsIndex> buildIndices(String[] indexNames, String javaRegex,
if (typeEntry.getKey().startsWith("_") && typeCap.getType().startsWith("_")) {
continue;
}

// skip the "flattened" type of field's "_keyed" subfield
Copy link
Member

@costin costin Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (fieldName.endsWith("." + FLATTENED_FIELD_SUFFIX) && typeEntry.getKey().equals(FlatObjectFieldMapper.CONTENT_TYPE)
&& typeCap.getType().equals(FlatObjectFieldMapper.CONTENT_TYPE)) {
continue;
}

// compute the actual indices - if any are specified, take into account the unmapped indices
List<String> concreteIndices = null;
Expand Down
29 changes: 28 additions & 1 deletion ...ugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/index/IndexResolverTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,33 @@ public void testMetaFieldsAreIgnored() throws Exception {
assertEquals(DataType.INTEGER, esIndex.mapping().get("_meta_field").getDataType());
assertEquals(DataType.KEYWORD, esIndex.mapping().get("text").getDataType());
}

public void testFlattenedHiddenSubfieldIsIgnored() throws Exception {
Map<String, Map<String, FieldCapabilities>> fieldCaps = new HashMap<>();
addFieldCaps(fieldCaps, "some_field", "flattened", false, false);
addFieldCaps(fieldCaps, "some_field._keyed", "flattened", false, false);
addFieldCaps(fieldCaps, "another_field", "object", true, false);
addFieldCaps(fieldCaps, "another_field._keyed", "keyword", true, false);
addFieldCaps(fieldCaps, "nested_field", "object", false, false);
addFieldCaps(fieldCaps, "nested_field.sub_field", "flattened", true, true);
addFieldCaps(fieldCaps, "nested_field.sub_field._keyed", "flattened", true, true);
addFieldCaps(fieldCaps, "text", "keyword", true, true);

String wildcard = "*";
IndexResolution resolution = IndexResolver.mergedMappings(wildcard, new String[] { "index" }, fieldCaps);
assertTrue(resolution.isValid());

EsIndex esIndex = resolution.get();
assertEquals(wildcard, esIndex.name());
assertNull(esIndex.mapping().get("some_field").getProperties().get("_keyed"));
assertNull(esIndex.mapping().get("nested_field").getProperties().get("sub_field").getProperties().get("_keyed"));
assertEquals(DataType.UNSUPPORTED, esIndex.mapping().get("some_field").getDataType());
assertEquals(DataType.OBJECT, esIndex.mapping().get("nested_field").getDataType());
assertEquals(DataType.UNSUPPORTED, esIndex.mapping().get("nested_field").getProperties().get("sub_field").getDataType());
assertEquals(DataType.KEYWORD, esIndex.mapping().get("text").getDataType());
assertEquals(DataType.OBJECT, esIndex.mapping().get("another_field").getDataType());
assertEquals(DataType.KEYWORD, esIndex.mapping().get("another_field").getProperties().get("_keyed").getDataType());
}

public void testMergeIncompatibleCapabilitiesOfObjectFields() throws Exception {
Map<String, Map<String, FieldCapabilities>> fieldCaps = new HashMap<>();
Expand Down Expand Up @@ -327,7 +354,7 @@ private static <K, V> void assertEqualsMaps(Map<K, V> left, Map<K, V> right) {
private void addFieldCaps(Map<String, Map<String, FieldCapabilities>> fieldCaps, String name, String type, boolean isSearchable,
boolean isAggregatable) {
Map<String, FieldCapabilities> cap = new HashMap<>();
cap.put(name, new FieldCapabilities(name, type, isSearchable, isAggregatable, Collections.emptyMap()));
cap.put(type, new FieldCapabilities(name, type, isSearchable, isAggregatable, Collections.emptyMap()));
fieldCaps.put(name, cap);
}
}