SQL: Add ability to perform CCS through SQL querying #78903

bpintea · 2021-10-11T06:57:40Z

This adds the ability to perform cross cluster searching through SQL.
This is done by simply using the CCS-specific cluster:index notation,
where the "cluster" maps to an SQL catalog and "index" to a table;
patterns are supported for both. Noteworthy here is that a search runs
either on the local or remote clusters, but not both (which is a
CCS-specific behavior).

Another limitation is currently that table enumeration in remote
clusters will not contain aliases (not reported through field caps).
Also, frozen indices are currently not listed as such (they're listed as
normal indices).

SYS COLUMNS command will not take a catalog pattern - an exact match is
required -, following xDBC spec. SYS TABLES can, however.

The query API now takes a new parameter, catalog, whose value is prefixed
to the index in the query, before execution, if the index is not already qualified
(which takes precedence).

This adds the ability to perform cross cluster searching through SQL. This is done by simply using the CCS-specific `cluster:index` notation, where the "cluster" maps to an SQL catalog and "index" to a table; patterns are supported for both. Noteworthy here is that a search runs either on the local or remote clusters, but not both (which is a CCS-specific behavior). Another limitation is currently that table enumeration in remote clusters will not contain aliases (not reported through field caps). Also, frozen indices are currently not listed as such (they're listed as normal indices). `SYS COLUMNS` command will not take a catalog pattern - an exact match is required -, following xDBC spec. `SYS TABLES` can, however.

License and WS fixes. Add a comment. Revert a debugging change.

bpintea · 2021-10-11T08:03:09Z

@elasticmachine update branch

Add getClusters() metadata test. Adjust expected output of other 'SHOW TABLES' tests.

elasticmachine · 2021-10-11T14:27:56Z

Pinging @elastic/es-ql (Team:QL)

...src/test/java/org/elasticsearch/xpack/sql/qa/multi_cluster_with_security/JdbcMetadataIT.java

costin

Gave a first round of feedback. Looks good overall.
It's not clear to me what test are running against a catalog and which one are not.
SELECT FROM table
vs
SELECT FROM local:table
vs
SELECT FROM remote:table

costin · 2021-10-13T16:44:01Z

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

        this.client = client;
        this.clusterName = clusterName;
        this.typeRegistry = typeRegistry;
+        this.remoteClusters = new RemoteClusterResolver(settings, clusterSettings);


Testing wise I think it's better to pass a provider that returns a Set vs the ClusterSettings which is an implementation detail.

I've pulled this class out as standalone and passed IndexResolver just a Supplier of the set.

costin · 2021-10-13T16:46:19Z

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

-            client.admin().indices().getAliases(aliasRequest, wrap(aliases ->
-                            resolveIndices(indices, javaRegex, aliases, retrieveIndices, retrieveFrozenIndices, listener),
-                            ex -> {
-                                // with security, two exception can be thrown:
-                                // INFE - if no alias matches
-                                // security exception is the user cannot access aliases
-
-                                // in both cases, that is allowed and we continue with the indices request
-                                if (ex instanceof IndexNotFoundException || ex instanceof ElasticsearchSecurityException) {
-                                    resolveIndices(indices, javaRegex, null, retrieveIndices, retrieveFrozenIndices, listener);
-                                } else {
-                                    listener.onFailure(ex);
+            client.admin().indices().getAliases(aliasRequest, wrap(aliases -> {


Can you comment on what's the difference between the old code and the new one - the whole block was formatted so it's difficult to keep track.

The significant changes are: (1) decouple and defer the output filtering from index/alias collection and (2) selectively cascade the resolution: aliases -> local indices -> remote indices (-> filtering), as these are needed.
The aliases and local indices resolution haven't otherwise changed.

costin · 2021-10-13T16:53:41Z

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

+                client.admin().indices().getIndex(indexRequest, wrap(indices -> {
+                        if (indices != null) {
+                            for (String indexName : indices.getIndices()) {
+                                boolean isFrozen = retrieveFrozenIndices
+                                    && indices.getSettings().get(indexName).getAsBoolean("index.frozen", false);
+                                indexInfos.add(new IndexInfo(clusterName, indexName,
+                                    isFrozen ? IndexType.FROZEN_INDEX : IndexType.STANDARD_INDEX));
+                            }
+                        }
+                        resolveRemoteIndices(clusterWildcard, indexWildcard, javaRegex, retrieveFrozenIndices, indexInfos, listener);
+                    },
+                    listener::onFailure)
+                );
+            } else {
+                resolveRemoteIndices(clusterWildcard, indexWildcard, javaRegex, retrieveFrozenIndices, indexInfos, listener);


This piece of code is similar to the method below (resolveRemoteIndices) - can the two be merged somehow?

I also thought about it, but didn't come to a solution. The flow is similar, but different request types and API and a "generified" would be more verbose eventually. But happy to apply suggestions.

costin · 2021-10-13T16:56:48Z

.../src/test/java/org/elasticsearch/xpack/sql/qa/multi_cluster_with_security/JdbcCsvSpecIT.java

+            sb.append(query.substring(i, j).replaceAll("(?i)(FROM)(\\s+)(\\w+|\"[^\"]+\")",
+                "$1$2" + buildRemoteIndexName(REMOTE_CLUSTER_NAME, "$3")));


Extract RemoteClusterResolver into own class and ony pass a supplier of remote clusters to the IndexResolver.

This adds a new "catalog" request parameter, to be used by the xDBC clients. It's value is whatever the set-catalog xDBC action is passed to by the application and will have ES/SQL run the query on the indicated cluster, in case the query itself doesn't spacify a cluster target (which takes precedence over the parameter).

bpintea · 2021-10-14T10:06:19Z

@elasticmachine update branch

Spotless reformat a file.

bpintea · 2021-10-14T11:18:53Z

It's not clear to me what test are running against a catalog and which one are not.
SELECT FROM table
vs
SELECT FROM local:table
vs
SELECT FROM remote:table

The JDBC driver now takes a new parameter, catalog, whose value is prefixed to the index, if this is not already qualified.

The JdbcCsvSpecIT test will randomly alternate between SELECT FROM table (with a "catalog": "remote" param) and SELECT FROM remote:table. Currently all the data is indexed in the remote cluster only.

astefan

Looks good overall. Left some minor comments.

astefan · 2021-10-14T15:06:54Z

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


        // first get aliases (if specified)
        boolean retrieveAliases = CollectionUtils.isEmpty(types) || types.contains(IndexType.ALIAS);
        boolean retrieveIndices = CollectionUtils.isEmpty(types) || types.contains(IndexType.STANDARD_INDEX);
        boolean retrieveFrozenIndices = CollectionUtils.isEmpty(types) || types.contains(IndexType.FROZEN_INDEX);

-        String[] indices = Strings.commaDelimitedListToStringArray(indexWildcard);


I am not sure why you removed this. Now you are explicitly calling Strings.commaDelimitedListToStringArray(indexWildcard) here and here.

I've restored it. In a previous design, the non-split version was called in 3 places, but now it makes sense to have it split at the beginning. (Renamed it tho, to be able to use the same name across invocations with no conflict of not-changed code.)

astefan · 2021-10-14T15:36:34Z

.../sql/qa/server/src/main/resources/multi-cluster-with-security/multi-cluster-command.csv-spec

+;
+
+showTablesWithFrozen
+SHOW TABLES CATALOG 'my_remote_cluster' INCLUDE FROZEN;


astefan · 2021-10-14T15:45:55Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java

@@ -179,7 +179,7 @@ private static FieldAttribute getFieldAttribute(String name) {
    }

    public void testPruneSubQueryAliases() {
-        ShowTables s = new ShowTables(EMPTY, null, null, false);
+        ShowTables s = new ShowTables(EMPTY, null, null, null,null, false);


Thanks, fixed.

costin · 2021-10-14T11:53:39Z

...sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/ConnectionConfiguration.java

@@ -65,9 +65,13 @@
    // NB: this is password instead of pass since that's what JDBC DriverManager/tools use
    public static final String AUTH_PASS = "password";

+    // Default catalog
+
+    public static final String CATALOG = "catalog";


why public?

costin · 2021-10-14T11:57:21Z

...sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/ConnectionConfiguration.java

+    // Default catalog
+
+    public static final String CATALOG = "catalog";
+
    protected static final Set<String> OPTION_NAMES = new LinkedHashSet<>(
            Arrays.asList(PROPERTIES_VALIDATION, BINARY_COMMUNICATION, CONNECT_TIMEOUT, NETWORK_TIMEOUT, QUERY_TIMEOUT, PAGE_TIMEOUT,


Why is the catalog defined on the connection configuration? This is about doing a network connection to the cluster not about its content.

The value of the catalog parameter will contain what the drivers receive from clients through the setCatalog()/SQL_ATTR_CURRENT_CATALOG (/USE/SET database potentially for CLI, in the future), i.e. the context in which to execute a query if it contains no catalog itself.

costin · 2021-10-14T11:59:21Z

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

@@ -256,6 +264,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
        if (zoneId != null) {
            builder.field(TIME_ZONE_NAME, zoneId.getId());
        }
+        if (catalog != null) {


Why add this code here? Not at the beginning nor at the end of the code block?

I tried to consistently keep the catalog after zoneId (member declaration, parameter order, code inserts), since catalog seems as "connection relevant" as zoneId (i.e. not really a meta config, but rather execution-relevant).

Change member access level, spacing.

Revert to splitting index string higher up in the execution.

astefan

Left a second round of comments/questions.

astefan · 2021-10-18T12:48:35Z

x-pack/plugin/sql/qa/server/multi-cluster-with-security/build.gradle

+apply plugin: 'elasticsearch.rest-test'
+
+dependencies {
+  testImplementation project(path: xpackModule('eql:qa:common'))


Why is eql here needed? Most likely it shouldn't be needed as dependency.

Well spotted. Replaced the dependency with a minimal one (ql:test-fixtures).

astefan · 2021-10-18T12:49:36Z

x-pack/plugin/sql/qa/server/multi-cluster-with-security/build.gradle

+  setting 'node.roles', '[data,ingest,master]'
+  setting 'xpack.ml.enabled', 'false'
+  setting 'xpack.watcher.enabled', 'false'
+  setting 'xpack.security.enabled', 'false'


security is disabled but the test project is called multi-cluster-with-security and you set an username and password later in the file. Doesn't look right.

Enabled back, thanks.

astefan · 2021-10-18T12:49:58Z

x-pack/plugin/sql/qa/server/multi-cluster-with-security/build.gradle

+    remoteClusterReg.get().getAllTransportPortURI().collect { "\"$it\"" }.toString()
+  }
+  setting 'cluster.remote.connections_per_cluster', "1"
+  setting 'xpack.security.enabled', 'false'


astefan · 2021-10-18T12:51:54Z

.../src/test/java/org/elasticsearch/xpack/sql/qa/multi_cluster_with_security/JdbcCatalogIT.java

+            assertFalse(rs.next());
+
+            es.setCatalog(LOCAL_CLUSTER_NAME);
+            expectThrows(SQLException.class, ps::executeQuery);


Is it worth checking for the error message as well?

astefan · 2021-10-18T13:06:01Z

.../src/test/java/org/elasticsearch/xpack/sql/qa/multi_cluster_with_security/JdbcCsvSpecIT.java

+        super(fileName, groupName, testName, lineNumber, randomBoolean() ? qualifyFromClause(testCase) : testCase);
+    }
+
+    // qualify the query FROM clause with the cluster name, but (crudely) skip `EXTRACT(a FROM b)` calls.


Why skipping the EXTRACT function?

To avoid changing queries to an incorrect format like SELECT ... EXTRACT(YEAR cluster:FROM date) FROM cluster:index ....

astefan · 2021-10-18T13:14:58Z

.../qa/server/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/CliSecurityIT.java

                for (String table : tables) {
                    String line = null;
                    /*
                     * Security automatically creates either a `.security` or a
                     * `.security6` index but it might not have created the index
                     * by the time the test runs.
                     */
-                    while (line == null || line.startsWith(".security")) {
+                    while (line == null || line.contains("|.security")) {


Why this change if the comment says ".security" or ".security6"? Either leave it as is, or do also change the comment, because there is a slight contradiction now.

SHOW TABLES will now have as first column the catalog and the table will be second. So for both .security and .security6 indices, the listing will no longer startWith() that value, but the line will contain a |.security (and/or |.security6).

astefan · 2021-10-18T15:10:33Z

.../plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

+    private void cleanup() throws IOException {
+        try {
+            deleteTestIndex();
+        } catch (ResponseException e) {
+            // while the majority of tests use the index(...) test method, few create their own index
+            if (e.getResponse().getStatusLine().getStatusCode() != 404) {
+                throw e;
+            }
+        }
+    }


Why is this needed? I thought that ESRestTestCase already does this if preserveIndicesUponCompletion() is not overridden, which should not be the case.

preserveIndicesUponCompletion() is used to clear (or not) the local cluster. The indices in these tests are created on the remote cluster and only queried locally.

astefan · 2021-10-18T15:12:04Z

.../plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

@@ -255,6 +273,8 @@ public void testNextPageWithDatetimeAndTimezoneParam() throws IOException {
            expected,
            runSql(new StringEntity(cursor(cursor).mode(mode).toString(), ContentType.APPLICATION_JSON), StringUtils.EMPTY, mode)
        );
+
+        deleteIndex("test_date_timezone");


Same question about this operation. Why is it specifically needed if one of the base classes should do the cleanup in an @After method.

Same as above.

astefan · 2021-10-18T15:13:14Z

.../plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

-            () -> runSql(randomMode(), "SELECT * FROM test JOIN other"),
-            containsString("line 1:21: Queries with JOIN are not yet supported")
+            () -> runSql(randomMode(), "SELECT * FROM " + indexPattern("test") + " JOIN other"),
+            containsString(": Queries with JOIN are not yet supported")


The line is removed because of the index pattern that now can have the cluster name in front of it?

Correct. These tests are executed both on local cluster and with CCS so the offsets would differ.

Address review comments

Enable test cluster security

Propagate credentials to JdbcIntegrationTestCase hierarchy.

Remove no longer used imports.

astefan

LGTM

elasticsearchmachine · 2021-10-20T06:25:59Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 78903

This adds the ability to perform cross cluster searching through SQL. This is done by simply using the CCS-specific `cluster:index` notation, where the "cluster" maps to an SQL catalog and "index" to a table; patterns are supported for both. Noteworthy here is that a search runs either on the local or remote clusters, but not both (which is a CCS-specific behaviour). Another limitation is currently that table enumeration in remote clusters will not contain aliases (not reported through field caps). Also, frozen indices are currently not listed as such (they're listed as normal indices). `SYS COLUMNS` command will not take a catalog pattern - an exact match is required -, following xDBC spec. `SYS TABLES` can, however. This adds a new "catalog" request parameter, to be used by the xDBC clients. It's value is whatever the set-catalog xDBC action is passed to by the application and will have ES/SQL run the query on the indicated cluster, in case the query itself doesn't specify a cluster target (which takes precedence over the parameter). (cherry picked from commit 733837d)

) (#79536) * SQL: Add ability to perform CCS through SQL querying (#78903) This adds the ability to perform cross cluster searching through SQL. This is done by simply using the CCS-specific `cluster:index` notation, where the "cluster" maps to an SQL catalog and "index" to a table; patterns are supported for both. Noteworthy here is that a search runs either on the local or remote clusters, but not both (which is a CCS-specific behaviour). Another limitation is currently that table enumeration in remote clusters will not contain aliases (not reported through field caps). Also, frozen indices are currently not listed as such (they're listed as normal indices). `SYS COLUMNS` command will not take a catalog pattern - an exact match is required -, following xDBC spec. `SYS TABLES` can, however. This adds a new "catalog" request parameter, to be used by the xDBC clients. It's value is whatever the set-catalog xDBC action is passed to by the application and will have ES/SQL run the query on the indicated cluster, in case the query itself doesn't specify a cluster target (which takes precedence over the parameter). (cherry picked from commit 733837d) * Remove test cluster setting not avail in 7.x Remove 'xpack.security.autoconfiguration.enabled' setting.

The QL team added support for SQL via CCS to both 7.16 and 8.0 - #78903

elasticsearchmachine added the v8.0.0 label Oct 11, 2021

License and WS fixes

330fe9f

License and WS fixes. Add a comment. Revert a debugging change.

elasticmachine and others added 3 commits October 11, 2021 19:03

Merge branch 'master' into feat/sql_ccs

6b42ee0

Add metadata test and fix expected output of other

b6cf678

Add getClusters() metadata test. Adjust expected output of other 'SHOW TABLES' tests.

Merge branch 'master' into feat/sql_ccs

dce4191

bpintea added :Analytics/SQL SQL querying >feature v7.16.0 labels Oct 11, 2021

bpintea requested review from Luegg, astefan and costin October 11, 2021 14:27

bpintea marked this pull request as ready for review October 11, 2021 14:27

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Oct 11, 2021

bpintea commented Oct 11, 2021

View reviewed changes

...src/test/java/org/elasticsearch/xpack/sql/qa/multi_cluster_with_security/JdbcMetadataIT.java Outdated Show resolved Hide resolved

bpintea marked this pull request as draft October 13, 2021 07:33

costin reviewed Oct 13, 2021

View reviewed changes

bpintea added 2 commits October 14, 2021 10:46

Address review comment

2765414

Extract RemoteClusterResolver into own class and ony pass a supplier of remote clusters to the IndexResolver.

Merge branch 'master' into feat/sql_ccs

b515ac3

bpintea marked this pull request as ready for review October 14, 2021 10:08

Apply spotless on previously accepted code

32e5746

Spotless reformat a file.

astefan reviewed Oct 14, 2021

View reviewed changes

costin reviewed Oct 14, 2021

View reviewed changes

bpintea added 2 commits October 14, 2021 22:15

Address review comments

7a80682

Change member access level, spacing.

Merge branch 'master' into feat/sql_ccs

13cce3e

Address review comments

09bbf4a

Revert to splitting index string higher up in the execution.

bpintea mentioned this pull request Oct 18, 2021

Add catalog support elastic/elasticsearch-sql-odbc#297

Merged

astefan reviewed Oct 18, 2021

View reviewed changes

bpintea added 6 commits October 18, 2021 20:31

Merge branch 'master' into feat/sql_ccs

e37975a

Address review comments

6e337a3

Address review comments

Enable test cluster security

20f87b2

Enable test cluster security

Fix multi-cluster test authentication

44e2fe4

Propagate credentials to JdbcIntegrationTestCase hierarchy.

Merge branch 'master' into feat/sql_ccs

928198e

Style fix

62229d5

Remove no longer used imports.

astefan approved these changes Oct 20, 2021

View reviewed changes

bpintea added the auto-backport-and-merge Automatically create backport pull requests and merge when ready label Oct 20, 2021

bpintea merged commit 733837d into elastic:master Oct 20, 2021

bpintea deleted the feat/sql_ccs branch October 20, 2021 06:25

bpintea added the backport pending label Oct 20, 2021

bpintea added release highlight and removed backport pending labels Oct 20, 2021

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

bpintea mentioned this pull request Dec 3, 2021

SQL: JDBC: add support for a default catalog #79990

Closed

giladgal added a commit that referenced this pull request Dec 7, 2021

Update search-across-clusters.asciidoc

5d8ac4b

The QL team added support for SQL via CCS to both 7.16 and 8.0 - #78903

bpintea mentioned this pull request Jan 5, 2022

SQL: Cross Cluster Search (CCS) support #29831

Closed

bpintea mentioned this pull request Feb 10, 2022

SQL: add CCS capabilities #74961

Closed

		sb.append(query.substring(i, j).replaceAll("(?i)(FROM)(\\s+)(\\w+\|\"[^\"]+\")",
		"$1$2" + buildRemoteIndexName(REMOTE_CLUSTER_NAME, "$3")));

SQL: Add ability to perform CCS through SQL querying #78903

SQL: Add ability to perform CCS through SQL querying #78903

Conversation

bpintea commented Oct 11, 2021 • edited Loading

bpintea commented Oct 11, 2021

elasticmachine commented Oct 11, 2021

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

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

bpintea commented Oct 14, 2021

bpintea commented Oct 14, 2021

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

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

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

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 20, 2021

💔 Backport failed

bpintea commented Oct 11, 2021 •

edited

Loading