SQL: Add support for FROZEN indices #41558

costin · 2019-04-25T22:16:46Z

Allow querying of FROZEN indices both through dedicated SQL grammar
extension:

SELECT field FROM FROZEN index

and also through driver configuration parameter, namely:

index.include.frozen: true/false

Allow querying of FROZEN indices both through dedicated SQL grammar extension: > SELECT field FROM FROZEN index and also through driver configuration parameter, namely: > index.include.frozen: true/false Fix elastic#39390 Fix elastic#39377

elasticmachine · 2019-04-25T22:16:47Z

Pinging @elastic/es-search

costin · 2019-04-26T07:44:37Z

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

-            try (Connection csv = csvConnection(testCase); Connection es = esJdbc()) {
-                executeAndAssert(csv, es);
-            }
+        try (Connection csv = csvConnection(testCase); Connection es = esJdbc()) {


@matriv The previous hack was unnecessary - the conditional should be addressed in connectionProperties and further more, esJdbc() actually calls esJdbc(connectionProperties()) so the code was not only duplicated but also identical in results.

I don't get how this is addressed only for a specific file, in this case: time.csv-spec.

It is not but then again, it wasn't before.
In other words, the if ("time".equals(groupName)) does the right thing but calling esJdbc(connectionProperties()) is the same as calling esJdbc() meaning the if and else branch end up calling the same code path.

It's not the case, if you check again there are 2 different connectionProperties() involved there. and the one for the time sets the timezone to UTC where the other uses a random timezone.

@costin Did you check this ^^ ?

costin · 2019-04-26T07:45:17Z

x-pack/plugin/sql/qa/src/main/resources/command.csv-spec

-test_alias_emp     |VIEW          
-test_emp           |BASE TABLE     
-test_emp_copy      |BASE TABLE     
+       name        |     type   |  native   


Still thinking whether the third column should be native or something else (storage doesn't fit either) and mapping seems inappropriate.

I like native but how about native_type ?

I think native_type is more clear...

somewhat redundant - type, native type - plus should it be native_type or native type? A single word is much preferred.

Imho, the name of a previous column (type) is completely irrelevant to the name of the next one, and I believe it's more clear to have it as native_type. And I would for sure vote for the _ instead of a space: .

I opted for kind - one word, small (4 letters like the rest) and fairly accurate.

costin · 2019-04-26T07:46:54Z

.../plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java

@@ -33,12 +33,14 @@
    private static final ObjectParser<SqlQueryRequest, Void> PARSER = objectParser(SqlQueryRequest::new);
    static final ParseField COLUMNAR = new ParseField("columnar");
    static final ParseField FIELD_MULTI_VALUE_LENIENCY = new ParseField("field_multi_value_leniency");
+    static final ParseField INDEX_INCLUDE_FROZEN = new ParseField("index_include_frozen");


@bpintea this is a new parameter that drivers can pass in to allow all queries inside one connection to include frozen indices. If not, the user would have to manually mention this through INCLUDE FROZEN and FROZEN.

Opened elastic/elasticsearch-sql-odbc#151. Note that the parameter is false by default.

@bpintea thanks - I think there's an area that needs more exploring, namely SYS commands in particular that of types. Since a frozen index is still mapped as a BASE TABLE, the server needs to properly take the setting into account but I have to double check whether that's the case - just something to keep in mind.

costin · 2019-04-26T07:47:44Z

x-pack/plugin/sql/src/main/antlr/SqlBase.g4

-    | SHOW TABLES (tableLike=likePattern | tableIdent=tableIdentifier)?                                   #showTables
-    | SHOW COLUMNS (FROM | IN) (tableLike=likePattern | tableIdent=tableIdentifier)                       #showColumns
-    | (DESCRIBE | DESC) (tableLike=likePattern | tableIdent=tableIdentifier)                              #showColumns
+    | SHOW TABLES (INCLUDE FROZEN)? (tableLike=likePattern | tableIdent=tableIdentifier)?                 #showTables


This is the declarative approach for frozen which is useful when not using index.include.frozen (again . are used by drivers, _ is used in rest).

costin · 2019-04-26T07:49:30Z

x-pack/plugin/sql/src/main/antlr/SqlBase.g4

-    | (DESCRIBE | DESC) (tableLike=likePattern | tableIdent=tableIdentifier)                              #showColumns
+    | SHOW TABLES (INCLUDE FROZEN)? (tableLike=likePattern | tableIdent=tableIdentifier)?                 #showTables
+    | SHOW COLUMNS (INCLUDE FROZEN)? (FROM | IN) (tableLike=likePattern | tableIdent=tableIdentifier)     #showColumns
+    | (DESCRIBE | DESC) (INCLUDE FROZEN)? (tableLike=likePattern | tableIdent=tableIdentifier)            #showColumns
    | SHOW FUNCTIONS (likePattern)?                                                                       #showFunctions
    | SHOW SCHEMAS                                                                                        #showSchemas
    | SYS TABLES (CATALOG clusterLike=likePattern)?


Notice the SYS commands do not require INCLUDE FROZEN since these are meant only for drivers in which case the option is set through the configuration.

costin · 2019-04-26T07:52:10Z

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

@@ -408,14 +444,14 @@ public void resolveAsSeparateMappings(String indexWildcard, String javaRegex, Ac
    /**
     * Assemble an index-based mapping from the field caps (which is field based) by looking at the indices associated with
     * each field.
-     */
+                 */


Looks like formatting got affected in merging -will follow up.

matriv

Looks great overall! Left some minor comments.

Could we have a test also for the translate api on a frozen index?

matriv · 2019-05-02T08:41:33Z

x-pack/plugin/sql/qa/src/main/resources/command.csv-spec

- name:s        | type:s
-test_alias     |VIEW          
-test_alias_emp |VIEW          
+ name:s        | type:s | native:s


Minor, maybe take the opportunity and align the outputs in the test results.

matriv · 2019-05-02T08:45:45Z

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

@@ -23,9 +24,9 @@
    public static class PreAnalysis {
        public static final PreAnalysis EMPTY = new PreAnalysis(emptyList());

-        public final List<TableIdentifier> indices;
+        public final List<Tuple<TableIdentifier, Boolean>> indices;


I would add the frozen as a property in TableIdentifier instead, maybe even rename it to TableInfo.

TableIdentifier is mapped to parsing and has a well defined scope - frozen is a property of a table not of its id (which is just another property).
Since in the future we might have other properties (potentially remote from CCS), I've retrofitted the tuple into a TableInfo class inside the analysis package.

matriv · 2019-05-02T09:51:41Z

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


-        public static final EnumSet<IndexType> VALID = EnumSet.of(INDEX, ALIAS);
+        public static final EnumSet<IndexType> VALID = EnumSet.of(STANDARD_INDEX, ALIAS, FROZEN_INDEX);
+        public static final EnumSet<IndexType> VALID_WO_FROZEN = EnumSet.of(STANDARD_INDEX, ALIAS);


Minor suggestion: I would choose to name it plain VALID and the other VALID_WITH_FROZEN.

matriv · 2019-05-02T09:56:02Z

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

-                // and create a MultiTypeField
-
-                for (Entry<String, FieldCapabilities> type : types.entrySet()) {
+                    // build the error message


You've already spotted it, but just a reminder to revert the formatting changes.

astefan

LGTM in general. Left few comments.
Documentation will be added later?

astefan · 2019-05-02T14:20:23Z

x-pack/plugin/sql/qa/src/main/resources/command.csv-spec

-test_alias_emp     |VIEW          
-test_emp           |BASE TABLE     
-test_emp_copy      |BASE TABLE     
+       name        |     type   |  native   


I think native_type is more clear...

astefan · 2019-05-02T14:37:15Z

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


        private final String toSql;
+        private final String toNative;


Maybe a bit more descriptive as toNativeType?

astefan · 2019-05-02T14:49:17Z

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

            GetIndexRequest indexRequest = new GetIndexRequest()
                    .local(true)
                    .indices(indices)
                    .features(Feature.SETTINGS)
                    .includeDefaults(false)
                    .indicesOptions(INDICES_ONLY_OPTIONS);
+
+            // if frozen indices are request, make sure to update the request accordingly


typo: if frozen indices are request**ed**

astefan · 2019-05-02T14:59:14Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java

        SearchRequest search = client.prepareSearch(indices)
-                // always track total hits accurately
-                .setTrackTotalHits(true)


Why removing this one?

Good catch - merging error.

astefan · 2019-05-02T15:08:33Z

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

@@ -449,7 +464,8 @@ public boolean equals(Object obj) {
                && Objects.equals(fields, other.fields)
                && Objects.equals(aliases, other.aliases)
                && Objects.equals(sort, other.sort)
-                && Objects.equals(limit, other.limit);
+                && Objects.equals(limit, other.limit) && Objects.equals(trackHits, other.trackHits)


Each equals on its own line, to have a consistent approach?

…n-indices

costin · 2019-05-03T14:56:53Z

LGTM in general. Left few comments.
Documentation will be added later?

Yes, I'd like to keep it separate since it would be easy to iterate over it in the CI without running the whole suite.

Add more tests

costin · 2019-05-06T15:12:04Z

Currently blocked by #41795

matriv

LGTM

astefan

LGTM

costin · 2019-05-09T20:01:03Z

run elasticsearch-ci/1

Allow querying of FROZEN indices both through dedicated SQL grammar extension: > SELECT field FROM FROZEN index and also through driver configuration parameter, namely: > index.include.frozen: true/false Fix #39390 Fix #39377 (cherry picked from commit 2445a93)

Allow querying of FROZEN indices both through dedicated SQL grammar extension: > SELECT field FROM FROZEN index and also through driver configuration parameter, namely: > index.include.frozen: true/false Fix elastic#39390 Fix elastic#39377 (cherry picked from commit 2445a93)

Allow querying of FROZEN indices both through dedicated SQL grammar extension: > SELECT field FROM FROZEN index and also through driver configuration parameter, namely: > index.include.frozen: true/false Fix elastic#39390 Fix elastic#39377

SQL: Add support for FROZEN indices

b6a63ea

Allow querying of FROZEN indices both through dedicated SQL grammar extension: > SELECT field FROM FROZEN index and also through driver configuration parameter, namely: > index.include.frozen: true/false Fix elastic#39390 Fix elastic#39377

costin added >feature :Analytics/SQL SQL querying v8.0.0 v7.2.0 labels Apr 25, 2019

costin requested review from astefan and matriv April 25, 2019 22:16

costin requested a review from bpintea April 26, 2019 07:43

costin commented Apr 26, 2019

View reviewed changes

Consider frozen indices inside SysTables

dd822d6

matriv reviewed May 2, 2019

View reviewed changes

astefan reviewed May 2, 2019

View reviewed changes

bpintea mentioned this pull request May 2, 2019

DSN parameter: support for "index_include_frozen" elastic/elasticsearch-sql-odbc#151

Merged

costin added 5 commits May 3, 2019 11:33

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

f56649a

…n-indices

Address feedback

e1c02f2

update tests

e0334e4

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

6103d9b

…n-indices

Move frozen to dedicated folder

7bddf5c

costin added 2 commits May 6, 2019 18:09

Address feedback

cf63d3c

Add more tests

Merge branch 'master' into sql/frozen-indices

9d3be49

costin mentioned this pull request May 6, 2019

Docs: Documentation for the upcoming SQL support of frozen indices #41863

Merged

matriv approved these changes May 7, 2019

View reviewed changes

costin added 2 commits May 9, 2019 11:24

Merge branch 'master' into sql/frozen-indices

8ac59f8

Update test

ddf5509

astefan approved these changes May 9, 2019

View reviewed changes

costin merged commit 2445a93 into elastic:master May 9, 2019

costin deleted the sql/frozen-indices branch May 9, 2019 21:27

imotov added a commit that referenced this pull request May 13, 2019

Fix tests to work with #41558

881a357

matriv mentioned this pull request May 24, 2019

SQL: Backport support for Frozen Indices #42505

Closed

costin added v7.3.0 and removed v7.2.0 labels May 25, 2019

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

SQL: Add support for FROZEN indices #41558

SQL: Add support for FROZEN indices #41558

Conversation

costin commented Apr 25, 2019 • edited Loading

elasticmachine commented Apr 25, 2019

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 Apr 26, 2019 • 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

matriv May 3, 2019 • 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

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

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

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

costin commented May 3, 2019

costin commented May 6, 2019

matriv left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

costin commented May 9, 2019

costin commented Apr 25, 2019 •

edited

Loading

costin Apr 26, 2019 •

edited

Loading

matriv May 3, 2019 •

edited

Loading