Cross cluster search in PPL #1512

seankao-az · 2023-04-10T22:08:47Z

Description

Support cross cluster search in PPL.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

ppl/src/main/antlr/OpenSearchPPLParser.g4

penghuo · 2023-04-11T00:25:19Z

ppl/src/main/antlr/OpenSearchPPLParser.g4

@@ -590,6 +600,10 @@ qualifiedName
    : ident (DOT ident)*                             #identsAsQualifiedName
    ;

+clusterQualifiedName


the other to do this would be to allow the use of : in index names.

I've considered this option. The reason why I didn't think it work very well is because we really only want : in search commands. We'll need to reject the : symbol else where such as describe command.

I will take this into consideration. Which do you think is better?

allowed use of : in index name

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequest.java

codecov-commenter · 2023-04-19T01:38:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.16%. Comparing base (3fc11a4) to head (04d03ea).
Report is 344 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #1512   +/-   ##
=========================================
  Coverage     97.16%   97.16%           
- Complexity     4116     4120    +4     
=========================================
  Files           371      371           
  Lines         10366    10373    +7     
  Branches        704      704           
=========================================
+ Hits          10072    10079    +7     
  Misses          287      287           
  Partials          7        7

Flag	Coverage Δ
sql-engine	`97.16% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Yury-Fridlyand · 2023-04-19T02:17:02Z

Can I use this for SQL ITs? How can I configure number of nodes? How can I set per-node settings in IT?
This will be very helpful for pagination ITs.

seankao-az · 2023-04-19T02:40:53Z

Can I use this for SQL ITs?

Yes if it's based on OpenSearchSQLRestTestCase

How can I configure number of nodes?

Set numberOfNodes in gradle build script for the cluster, for example:

sql/integ-test/build.gradle

Line 310 in 16b900c

numberOfNodes = 3

How can I set per-node settings in IT?

I'm not sure about per-node settings...
But for cluster settings we have this function:

sql/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java

Line 353 in 16b900c

    
           protected static JSONObject updateClusterSettings(ClusterSetting setting) throws IOException {

in this PR I allow it to also update remote cluster settings

Yury-Fridlyand · 2023-04-19T16:38:28Z

Thanks!

docs/user/ppl/admin/cross_cluster_search.rst

penghuo · 2023-04-21T16:33:40Z

docs/user/ppl/admin/security.rst

@@ -34,7 +34,8 @@ Example: Create the ppl_role for test_user. then test_user could use PPL to quer
        ],
        "allowed_actions": [
          "indices:data/read/search*",
-          "indices:admin/mappings/get"
+          "indices:admin/mappings/get",
+          "indices:monitor/settings/get"


could you explain more why this permission is required?

I found during testing that this permission is needed. I believe it's this part to get cluster max result window setting that needs this permission.

I shall remove this change and make a separate PR for it, as it's not directly related to this PR

It is ok to add to this PR.
Please create an follow up issue to create the PPL_ACCESS in security plugin.

added back to the doc. will make a PR to security plugin to create roles for both ppl and ccs

ppl/src/main/antlr/OpenSearchPPLParser.g4

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequest.java

docs/user/ppl/admin/cross_cluster_search.rst

Signed-off-by: Sean Kao <[email protected]>

docs/user/ppl/admin/cross_cluster_search.rst

docs/user/ppl/general/identifiers.rst

ppl/src/main/antlr/OpenSearchPPLLexer.g4

opensearch-trigger-bot · 2023-05-12T16:15:37Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1512-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1bda5fe73312017b3bc6029e2eaf378c2e72306c
# Push it to GitHub
git push --set-upstream origin backport/backport-1512-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1512-to-2.x.

* feat: PPL parser for ccs Signed-off-by: Sean Kao <[email protected]> * feat: disable describe remote cluster index in PPL Allowing the syntax will lead to misunderstanding in the query result, because we will do a local cluster query for index mapping, even for remote indices. This is due to the restriction that OpenSearch doesn't support remote cluster index mapping query at the moment. Signed-off-by: Sean Kao <[email protected]> * feat: Query system index without cluster name We require system index query to happen at the local cluster. Currently, OpenSearch does not support cross cluster system index query. Thus, mapping of a remote index is unavailable. Therefore, we require the local cluster to have the system index of the remote cluster index. The full "cluster:index" name is still used to query OpenSearch for datarows, as CCS is natively supported. Signed-off-by: Sean Kao <[email protected]> * fix: index name parsing for datasources To identify datasources in the index qualified names, they need to be parsed into parts (separated only by dots). clusterQualifiedName can't contain custom datasources, hence the distinction. Signed-off-by: Sean Kao <[email protected]> * multi clusters setup for integration test Signed-off-by: Sean Kao <[email protected]> * Add IT test case Signed-off-by: Sean Kao <[email protected]> * Document ccs for ppl Signed-off-by: Sean Kao <[email protected]> * documentation update Signed-off-by: Sean Kao <[email protected]> * feat: allow describe remote cluster index in PPL Signed-off-by: Sean Kao <[email protected]> * feat: allow "*:index" to match all remote clusters Signed-off-by: Sean Kao <[email protected]> * use local index names for field mappings request Signed-off-by: Sean Kao <[email protected]> * allow ':' in index identifier Signed-off-by: Sean Kao <[email protected]> * docs update Signed-off-by: Sean Kao <[email protected]> * limit cluster prefix to table names only Signed-off-by: Sean Kao <[email protected]> * move multicluster capability to sql rest test case Signed-off-by: Sean Kao <[email protected]> * add IT for failure case Signed-off-by: Sean Kao <[email protected]> * remove logger info for connection in IT test case Signed-off-by: Sean Kao <[email protected]> --------- Signed-off-by: Sean Kao <[email protected]> (cherry picked from commit 1bda5fe)

* feat: PPL parser for ccs Signed-off-by: Sean Kao <[email protected]> * feat: disable describe remote cluster index in PPL Allowing the syntax will lead to misunderstanding in the query result, because we will do a local cluster query for index mapping, even for remote indices. This is due to the restriction that OpenSearch doesn't support remote cluster index mapping query at the moment. Signed-off-by: Sean Kao <[email protected]> * feat: Query system index without cluster name We require system index query to happen at the local cluster. Currently, OpenSearch does not support cross cluster system index query. Thus, mapping of a remote index is unavailable. Therefore, we require the local cluster to have the system index of the remote cluster index. The full "cluster:index" name is still used to query OpenSearch for datarows, as CCS is natively supported. Signed-off-by: Sean Kao <[email protected]> * fix: index name parsing for datasources To identify datasources in the index qualified names, they need to be parsed into parts (separated only by dots). clusterQualifiedName can't contain custom datasources, hence the distinction. Signed-off-by: Sean Kao <[email protected]> * multi clusters setup for integration test Signed-off-by: Sean Kao <[email protected]> * Add IT test case Signed-off-by: Sean Kao <[email protected]> * Document ccs for ppl Signed-off-by: Sean Kao <[email protected]> * documentation update Signed-off-by: Sean Kao <[email protected]> * feat: allow describe remote cluster index in PPL Signed-off-by: Sean Kao <[email protected]> * feat: allow "*:index" to match all remote clusters Signed-off-by: Sean Kao <[email protected]> * use local index names for field mappings request Signed-off-by: Sean Kao <[email protected]> * allow ':' in index identifier Signed-off-by: Sean Kao <[email protected]> * docs update Signed-off-by: Sean Kao <[email protected]> * limit cluster prefix to table names only Signed-off-by: Sean Kao <[email protected]> * move multicluster capability to sql rest test case Signed-off-by: Sean Kao <[email protected]> * add IT for failure case Signed-off-by: Sean Kao <[email protected]> * remove logger info for connection in IT test case Signed-off-by: Sean Kao <[email protected]> --------- Signed-off-by: Sean Kao <[email protected]> Signed-off-by: Mitchell Gale <[email protected]>

penghuo reviewed Apr 11, 2023

View reviewed changes

YANG-DB previously approved these changes Apr 11, 2023

View reviewed changes

seankao-az dismissed YANG-DB’s stale review via 4f4d47d April 18, 2023 20:17

seankao-az force-pushed the feature/cross-cluster-search branch 2 times, most recently from 93d441c to 6ac5d70 Compare April 18, 2023 23:51

seankao-az force-pushed the feature/cross-cluster-search branch from 88bbcc1 to 7797ba7 Compare April 19, 2023 20:05

seankao-az marked this pull request as ready for review April 20, 2023 17:13

seankao-az requested review from anirudha, ps48, kavithacm, derek-ho, joshuali925, dai-chen, rupal-bq, mengweieric, vmmusings, Yury-Fridlyand and MaxKsyunz as code owners April 20, 2023 17:13

seankao-az requested review from penghuo and YANG-DB April 20, 2023 17:14

seankao-az force-pushed the feature/cross-cluster-search branch from 7797ba7 to d94322c Compare April 20, 2023 20:29

penghuo reviewed Apr 21, 2023

View reviewed changes

seankao-az force-pushed the feature/cross-cluster-search branch from d94322c to 38f4a15 Compare April 24, 2023 22:06

seankao-az requested a review from penghuo April 25, 2023 00:13

penghuo reviewed Apr 25, 2023

View reviewed changes

docs/user/ppl/admin/cross_cluster_search.rst Show resolved Hide resolved

penghuo reviewed Apr 25, 2023

View reviewed changes

docs/user/ppl/admin/cross_cluster_search.rst Outdated Show resolved Hide resolved

seankao-az added 5 commits April 27, 2023 12:08

feat: allow "*:index" to match all remote clusters

95f211b

Signed-off-by: Sean Kao <[email protected]>

use local index names for field mappings request

2d179a7

Signed-off-by: Sean Kao <[email protected]>

allow ':' in index identifier

572bdbb

Signed-off-by: Sean Kao <[email protected]>

docs update

a283a5b

Signed-off-by: Sean Kao <[email protected]>

limit cluster prefix to table names only

c03b69e

Signed-off-by: Sean Kao <[email protected]>

seankao-az force-pushed the feature/cross-cluster-search branch from 00410d3 to c03b69e Compare April 27, 2023 19:36

seankao-az added 2 commits April 27, 2023 13:10

move multicluster capability to sql rest test case

10d1ff6

Signed-off-by: Sean Kao <[email protected]>

add IT for failure case

30971b1

Signed-off-by: Sean Kao <[email protected]>

seankao-az force-pushed the feature/cross-cluster-search branch from 90a0e96 to 30971b1 Compare April 27, 2023 21:37

seankao-az requested a review from penghuo April 27, 2023 23:29

remove logger info for connection in IT test case

04d03ea

Signed-off-by: Sean Kao <[email protected]>

penghuo approved these changes May 1, 2023

View reviewed changes

anirudha approved these changes May 1, 2023

View reviewed changes

dai-chen added feature PPL Piped processing language labels May 1, 2023

YANG-DB requested changes May 1, 2023

View reviewed changes

docs/user/ppl/admin/cross_cluster_search.rst Show resolved Hide resolved

docs/user/ppl/general/identifiers.rst Show resolved Hide resolved

ppl/src/main/antlr/OpenSearchPPLLexer.g4 Show resolved Hide resolved

seankao-az requested a review from YANG-DB May 1, 2023 18:13

YANG-DB approved these changes May 2, 2023

View reviewed changes

seankao-az merged commit 1bda5fe into opensearch-project:main May 2, 2023

seankao-az mentioned this pull request May 4, 2023

[o11y] Cross-Cluster-Search support for PPL opensearch-project/observability#1431

Closed

seankao-az added the backport 2.x label May 12, 2023

seankao-az mentioned this pull request May 12, 2023

[Backport 2.x] Cross cluster search in PPL (#1512) #1627

Merged

6 tasks

Yury-Fridlyand mentioned this pull request May 25, 2023

Fix IT on 2.x - address breaking changes from upstream. #1659

Merged

6 tasks

acarbonetto added the v2.8.0 label May 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross cluster search in PPL #1512

Cross cluster search in PPL #1512

seankao-az commented Apr 10, 2023 •

edited

Loading

penghuo Apr 11, 2023

seankao-az Apr 11, 2023 •

edited

Loading

seankao-az Apr 24, 2023

codecov-commenter commented Apr 19, 2023 •

edited by codecov bot

Loading

Yury-Fridlyand commented Apr 19, 2023

seankao-az commented Apr 19, 2023

Yury-Fridlyand commented Apr 19, 2023

penghuo Apr 21, 2023

seankao-az Apr 21, 2023

penghuo Apr 26, 2023

seankao-az Apr 27, 2023 •

edited

Loading

opensearch-trigger-bot bot commented May 12, 2023

Cross cluster search in PPL #1512

Cross cluster search in PPL #1512

Conversation

seankao-az commented Apr 10, 2023 • edited Loading

Description

Issues Resolved

Check List

penghuo Apr 11, 2023

Choose a reason for hiding this comment

seankao-az Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

seankao-az Apr 24, 2023

Choose a reason for hiding this comment

codecov-commenter commented Apr 19, 2023 • edited by codecov bot Loading

Codecov Report

Yury-Fridlyand commented Apr 19, 2023

seankao-az commented Apr 19, 2023

Yury-Fridlyand commented Apr 19, 2023

penghuo Apr 21, 2023

Choose a reason for hiding this comment

seankao-az Apr 21, 2023

Choose a reason for hiding this comment

penghuo Apr 26, 2023

Choose a reason for hiding this comment

seankao-az Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented May 12, 2023

seankao-az commented Apr 10, 2023 •

edited

Loading

seankao-az Apr 11, 2023 •

edited

Loading

codecov-commenter commented Apr 19, 2023 •

edited by codecov bot

Loading

seankao-az Apr 27, 2023 •

edited

Loading