SQL: Support pattern against compatible indices by costin · Pull Request #34718 · elastic/elasticsearch

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

        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

        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

+                .indicesOptions(IndicesOptions.lenientExpandOpen());
    }

+    // 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

+                .indicesOptions(IndicesOptions.lenientExpandOpen());
    }

+    // 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

Conversation

costin commented Oct 22, 2018

Uh oh!

elasticmachine commented Oct 22, 2018

Uh oh!

astefan 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!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

costin commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

costin Oct 23, 2018 •

edited

Loading

costin commented Oct 23, 2018 •

edited

Loading