Add new setting to ignore pagination where it is unsupported in V2. #318

Yury-Fridlyand · 2023-08-01T02:49:35Z

Description

Changes how pagination requests with unsupported/incompatible [with pagination] queries are handled.
Adds new setting plugins.query.ignore_unsupported_pagination:

true by default
when true such requests are performed without pagination
when false such requests fall back to legacy engine

Misc changes:

Minor logging improvements
Minor doc grooming
Use pagination in correctness test

curl -X PUT localhost:9200/_cluster/settings?pretty -H "Content-Type: application/json" -d '{ "transient": { "plugins.query.ignore_unsupported_pagination": true } }'

Issues Resolved

opensearch-project#1765, opensearch-project#78

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.

codecov · 2023-08-01T02:53:20Z

Codecov Report

❗ No coverage uploaded for pull request base (integ-ignore-pagination-switch@bb3c340). Click here to learn what that means.
The diff coverage is n/a.

@@                        Coverage Diff                        @@
##             integ-ignore-pagination-switch     #318   +/-   ##
=================================================================
  Coverage                                  ?   97.52%           
  Complexity                                ?     4656           
=================================================================
  Files                                     ?      408           
  Lines                                     ?    11963           
  Branches                                  ?      830           
=================================================================
  Hits                                      ?    11667           
  Misses                                    ?      289           
  Partials                                  ?        7

Flag	Coverage Δ
sql-engine	`97.52% <0.00%> (?)`

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

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

plugin/src/main/java/org/opensearch/sql/plugin/config/OpenSearchPluginModule.java

matthewryanwells · 2023-08-01T15:53:38Z

Maybe update the PR description to include an example showing how you change/set the new setting. Also I noticed how the setting can be changed when making an opensearch request but how would I change the setting if I wanted to make it through the opensearchsql plugin?

Yury-Fridlyand · 2023-08-01T16:38:11Z

Updated

acarbonetto · 2023-08-01T16:25:17Z

common/src/main/java/org/opensearch/sql/common/setting/Settings.java

@@ -41,6 +41,7 @@ public enum Key {
    QUERY_SIZE_LIMIT("plugins.query.size_limit"),
    ENCYRPTION_MASTER_KEY("plugins.query.datasources.encryption.masterkey"),
    DATASOURCES_URI_ALLOWHOSTS("plugins.query.datasources.uri.allowhosts"),
+    IGNORE_UNSUPPORTED_PAGINATION("plugins.query.ignore_unsupported_pagination"),


does this only affect sql request? Maybe plugins.sql.ignore_unsupported_pagination or plugins.sql.cursor.ignore_unsupported?

It affects all pagination requests, but we support pagination in SQL only for now.
I can easily rename the setting.

Signed-off-by: Yury-Fridlyand <[email protected]>

acarbonetto · 2023-08-18T17:41:46Z

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

@@ -392,7 +392,7 @@ protected static JSONObject updateClusterSettings(ClusterSetting setting, RestCl
    return new JSONObject(executeRequest(request, client));
  }

-  protected static JSONObject updateClusterSettings(ClusterSetting setting) throws IOException {
+  public static JSONObject updateClusterSettings(ClusterSetting setting) throws IOException {


why does this need to be made public? It's being called by ITs that extend this class.

I use it in correctness test, which does not inherit this class.

acarbonetto · 2023-08-18T17:41:56Z

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

@@ -404,7 +404,7 @@ protected static JSONObject getAllClusterSettings() throws IOException {
    return new JSONObject(executeRequest(request));
  }

-  protected static class ClusterSetting {
+  public static class ClusterSetting {


I use it in correctness test, which does not inherit this class.

acarbonetto · 2023-08-18T17:52:09Z

integ-test/src/test/java/org/opensearch/sql/sql/PaginationFallbackIT.java

+                l -> l.endsWith("Request is not supported and falling back to old SQL engine")));
+    logSize = lines.size();
+
+    updateClusterSettings(


feels like these should be two different tests

acarbonetto · 2023-08-18T17:52:36Z

integ-test/src/test/java/org/opensearch/sql/sql/SQLCorrectnessIT.java

@@ -23,6 +28,9 @@ public class SQLCorrectnessIT extends CorrectnessTestBase {
  @Override
  protected void init() throws Exception {
    super.init();
+    updateClusterSettings(


is this needed? I feel like this is unnecessary for the tests to pass.

It it true by default, but previous test may set it to false.

acarbonetto · 2023-08-18T17:53:00Z

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

@@ -155,8 +155,8 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
                            if (newSqlRequest.isExplainRequest()) {
                                LOG.info("Request is falling back to old SQL engine due to: " + exception.getMessage());
                            }
-                            LOG.info("[{}] Request {} is not supported and falling back to old SQL engine",
-                                QueryContext.getRequestId(), newSqlRequest);
+                            LOG.info("[{}] Request is not supported and falling back to old SQL engine",


It was dumping non anonymized query

acarbonetto · 2023-08-18T17:55:50Z

docs/user/index.rst

@@ -3,7 +3,7 @@
 OpenSearch SQL Reference Manual
 ===============================

-OpenSearch SQL enables you to extract insights out of OpenSearch using the familiar SQL query syntax. Please refer to the `technical documentation <https://docs-beta.opensearch.org/>`_ for detailed information on installing and configuring opendistro-elasticsearch-sql plugin. In this user reference manual, you can find many information for your reference. In each part, we try to make it clear by adding work example along with detailed description. Here is table of contents of the documentation:
+OpenSearch SQL enables you to extract insights out of OpenSearch using the familiar SQL query syntax. Please refer to the `technical documentation <https://opensearch.org/docs/latest/install-and-configure/plugins/>`_ for detailed information on installing and configuring SQL plugin. In this user reference manual, you can find many information for your reference. In each part, we try to make it clear by adding work example along with detailed description. Here is table of contents of the documentation:


Yury-Fridlyand requested review from MitchellGale and matthewryanwells August 1, 2023 02:49

Yury-Fridlyand requested review from MaxKsyunz, forestmvey, acarbonetto and GumpacG as code owners August 1, 2023 02:49

matthewryanwells reviewed Aug 1, 2023

View reviewed changes

plugin/src/main/java/org/opensearch/sql/plugin/config/OpenSearchPluginModule.java Outdated Show resolved Hide resolved

matthewryanwells approved these changes Aug 1, 2023

View reviewed changes

GumpacG approved these changes Aug 1, 2023

View reviewed changes

acarbonetto reviewed Aug 1, 2023

View reviewed changes

Yury-Fridlyand force-pushed the dev-ignore-pagination-switch branch from 165e98c to 2d33f53 Compare August 15, 2023 02:45

Yury-Fridlyand mentioned this pull request Aug 15, 2023

Rework ComparisonIT to use pagination. #254

Closed

6 tasks

acarbonetto approved these changes Aug 17, 2023

View reviewed changes

Yury-Fridlyand added 3 commits August 17, 2023 16:45

Add new setting to ignore pagination where it is unsupported in V2.

796b7cf

Signed-off-by: Yury-Fridlyand <[email protected]>

Use pagination in correctess tests too.

eda6280

Signed-off-by: Yury-Fridlyand <[email protected]>

spotless

9ba8341

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand force-pushed the dev-ignore-pagination-switch branch from 2d33f53 to 9ba8341 Compare August 17, 2023 23:49

Fix tests.

662ad07

Signed-off-by: Yury-Fridlyand <[email protected]>

acarbonetto reviewed Aug 18, 2023

View reviewed changes

Yury-Fridlyand merged commit 1bb815b into integ-ignore-pagination-switch Aug 18, 2023
7 of 20 checks passed

Yury-Fridlyand deleted the dev-ignore-pagination-switch branch August 18, 2023 21:57

Yury-Fridlyand mentioned this pull request Aug 18, 2023

Add new setting to ignore pagination where it is unsupported in V2. opensearch-project/sql#1997

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new setting to ignore pagination where it is unsupported in V2. #318

Add new setting to ignore pagination where it is unsupported in V2. #318

Yury-Fridlyand commented Aug 1, 2023 •

edited

Loading

codecov bot commented Aug 1, 2023 •

edited

Loading

matthewryanwells commented Aug 1, 2023

Yury-Fridlyand commented Aug 1, 2023

acarbonetto Aug 1, 2023

Yury-Fridlyand Aug 1, 2023

acarbonetto Aug 18, 2023

Yury-Fridlyand Aug 18, 2023

acarbonetto Aug 18, 2023

Yury-Fridlyand Aug 18, 2023

acarbonetto Aug 18, 2023

acarbonetto Aug 18, 2023

Yury-Fridlyand Aug 18, 2023

acarbonetto Aug 18, 2023

Yury-Fridlyand Aug 18, 2023

acarbonetto Aug 18, 2023

Add new setting to ignore pagination where it is unsupported in V2. #318

Add new setting to ignore pagination where it is unsupported in V2. #318

Conversation

Yury-Fridlyand commented Aug 1, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Aug 1, 2023 • edited Loading

Codecov Report

matthewryanwells commented Aug 1, 2023

Yury-Fridlyand commented Aug 1, 2023

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

Yury-Fridlyand commented Aug 1, 2023 •

edited

Loading

codecov bot commented Aug 1, 2023 •

edited

Loading