SQL: Support pattern against compatible indices #34718

costin · 2018-10-22T19:11:19Z

Extend querying support on multiple indices from being strictly
identical to being just compatible.
Use FieldCapabilities API (extended through #33803) for mapping merging.

Close #31837 #31611

Extend querying support on multiple indices from being strictly identical to being just compatible. Use FieldCapabilities API (extended through elastic#33803) for mapping merging. Close elastic#31837 elastic#31611

elasticmachine · 2018-10-22T20:30:53Z

Pinging @elastic/es-search-aggs

astefan

LGTM. Left some really minor comments, mostly based on picky concerns.

astefan · 2018-10-23T05:15:13Z

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

+                }
+                if (fieldCap.isSearchable() && fieldCap.nonSearchableIndices() != null) {
+                    errorMessage.append("[" + indexPattern + "] points to indices with incompatible mappings: ");
+                    errorMessage.append("field [" + name + "] is searchable expect in ");


Typo for expect, should have been except.

astefan · 2018-10-23T05:37:44Z

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

+            case KEYWORD:
+                int length = DataType.KEYWORD.defaultPrecision;
+                boolean normalized = false;
+                field = new KeywordEsField(fieldName, props, caps.isAggregatable(), length, normalized);


Why just not passing false to the constructor, but using the normalized boolean?

Mainly to not forget about it - I've added a comment regarding its presence.

astefan · 2018-10-23T05:47:54Z

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

+        EsField field = null;
+        Map<String, EsField> props = hasChildren ? new TreeMap<>() : emptyMap();
+
+        // not currently present, means it's a parent field - currently just return it as an OBJECT


I don't understand this comment. This should be a valid statement when there is no dot (no subfields left), but the line it's put before of, doesn't indicate this assumption? Sorry if I missed anything.

Old comment, irrelevant. Removed it.

astefan · 2018-10-23T06:06:19Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ErrorsTestCase.java

@@ -40,7 +41,8 @@ public void testSelectFromIndexWithoutTypes() throws Exception {

        try (Connection c = esJdbc()) {
            SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT * FROM test").executeQuery());
-            assertEquals("Found 1 problem(s)\nline 1:15: [test] doesn't have any types so it is incompatible with sql", e.getMessage());
+            //assertEquals("Found 1 problem(s)\nline 1:15: [test] doesn't have any types so it is incompatible with sql", e.getMessage());


Why not deleting this line altogether?

astefan · 2018-10-23T06:06:36Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java

@@ -219,7 +219,8 @@ public void testSelectFromIndexWithoutTypes() throws Exception {
        client().performRequest(request);
        String mode = randomFrom("jdbc", "plain");
        expectBadRequest(() -> runSql(mode, "SELECT * FROM test"),
-            containsString("1:15: [test] doesn't have any types so it is incompatible with sql"));
+            //containsString("1:15: [test] doesn't have any types so it is incompatible with sql"));


…-to-field-caps

matriv

Looks Good!!

Minor and just a personal preference: I don't like so much the caps abbreviation for capabilities because it's commonly used for capitalisation. I don't have a good suggestion though... maybe capbs :-) ?

Also left a question for the error message testing.

matriv · 2018-10-23T10:05:33Z

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

    }

+    // Concrete indices still uses get mapping


maybe add a TODO to be more clear?

it's rather WIP blocked by : #34071
As this is high on my list, I'd like that as is instead of a TODO (which typically suggests uncompleted work) - fro 6.5.x there's no much we can do on this front...

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

matriv · 2018-10-23T10:15:32Z

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

    }

+    // Concrete indices still uses get mapping


Maybe add a TODO to be more clear?

matriv · 2018-10-23T10:19:05Z

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

+                        if (errorMessage.length() > 0) {
+                            errorMessage.append(", ");
+                        }
+                        errorMessage.append("[");


Is this tested with multiple entries?

If you're asking about the error message, yes in IndexResolverTests (e.g. testMergeIncompatibleTypes).

I was confused with the StringBuilder here, as you later on insert(0 for the prefix of the message.
You could use an ArrayList for the type.getKey() and a StringJoiner to append them to the StringBuilder.
Really minor, just a suggestion.

costin · 2018-10-23T10:59:12Z

@matriv fieldCaps is already used inside the public API and through-out the code. I guess it's just another case of overloading : see https://www.elastic.co/guide/en/elasticsearch/reference/current/search-field-caps.html (the API name is fieldCaps).

matriv

LGTM!!

Extend querying support on multiple indices from being strictly identical to being just compatible. Use FieldCapabilities API (extended through #33803) for mapping merging. Close #31837 #31611 (cherry picked from commit ca6808e)

* master: (24 commits) ingest: better support for conditionals with simulate?verbose (elastic#34155) [Rollup] Job deletion should be invoked on the allocated task (elastic#34574) [DOCS] .Security index is never auto created (elastic#34589) CCR: Requires soft-deletes on the follower (elastic#34725) re-enable bwc tests (elastic#34743) Empty GetAliases authorization fix (elastic#34444) INGEST: Document Processor Conditional (elastic#33388) [CCR] Add total fetch time leader stat (elastic#34577) SQL: Support pattern against compatible indices (elastic#34718) [CCR] Auto follow pattern APIs adjustments (elastic#34518) [Test] Remove dead code from ExceptionSerializationTests (elastic#34713) A small typo in migration-assistance doc (elastic#34704) ingest: processor stats (elastic#34724) SQL: Implement IN(value1, value2, ...) expression. (elastic#34581) Tests: Add checks to GeoDistanceQueryBuilderTests (elastic#34273) INGEST: Rename Pipeline Processor Param. (elastic#34733) Core: Move IndexNameExpressionResolver to java time (elastic#34507) [DOCS] Force Merge: clarify execution and storage requirements (elastic#33882) TESTING.asciidoc fix examples using forbidden annotation (elastic#34515) SQL: Implement `CONVERT`, an alternative to `CAST` (elastic#34660) ...

Extend querying support on multiple indices from being strictly identical to being just compatible. Use FieldCapabilities API (extended through #33803) for mapping merging. Close #31837 #31611

SQL: Support pattern against compatible indices

6d38f53

Extend querying support on multiple indices from being strictly identical to being just compatible. Use FieldCapabilities API (extended through elastic#33803) for mapping merging. Close elastic#31837 elastic#31611

costin added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.5.0 labels Oct 22, 2018

costin requested review from astefan and matriv October 22, 2018 19:11

astefan approved these changes Oct 23, 2018

View reviewed changes

costin added 2 commits October 23, 2018 11:20

Address feedback

ad787e4

Merge remote-tracking branch 'remotes/upstream/master' into migration…

53cacaa

…-to-field-caps

matriv reviewed Oct 23, 2018

View reviewed changes

More feedback

4046737

matriv approved these changes Oct 23, 2018

View reviewed changes

costin merged commit ca6808e into elastic:master Oct 23, 2018

costin deleted the migration-to-field-caps branch October 23, 2018 14:08

This was referenced Nov 2, 2018

SQL: using a normalized field in a GROUP BY throws an unexpected error #35200

Closed

SQL: normalized keywords shouldn't be allowed for groupings and sorting #35203

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

horacimacias mentioned this pull request May 10, 2019

VerificationException: When using multiple indices, the mappings must be identical. opendistro-for-elasticsearch/sql#46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Support pattern against compatible indices #34718

SQL: Support pattern against compatible indices #34718

costin commented Oct 22, 2018

elasticmachine commented Oct 22, 2018

astefan left a comment

astefan Oct 23, 2018

costin Oct 23, 2018

astefan Oct 23, 2018

costin Oct 23, 2018

astefan Oct 23, 2018

costin Oct 23, 2018

astefan Oct 23, 2018

costin Oct 23, 2018

astefan Oct 23, 2018

costin Oct 23, 2018

matriv left a comment

matriv Oct 23, 2018

costin Oct 23, 2018 •

edited

Loading

matriv Oct 23, 2018

matriv Oct 23, 2018

costin Oct 23, 2018

matriv Oct 23, 2018

costin commented Oct 23, 2018 •

edited

Loading

matriv left a comment

SQL: Support pattern against compatible indices #34718

SQL: Support pattern against compatible indices #34718

Conversation

costin commented Oct 22, 2018

elasticmachine commented Oct 22, 2018

astefan 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin Oct 23, 2018 • edited Loading

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

costin commented Oct 23, 2018 • edited Loading

matriv left a comment

Choose a reason for hiding this comment

costin Oct 23, 2018 •

edited

Loading

costin commented Oct 23, 2018 •

edited

Loading