Add new setting to ignore pagination where it is unsupported in V2. by Yury-Fridlyand · Pull Request #318 · Bit-Quill/opensearch-project-sql

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

    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 <yury.fridlyand@improving.com>

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

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

  }

-  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

  }

-  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

  @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

                            }
-                            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

 ===============================

-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 GumpacG, MaxKsyunz, acarbonetto and forestmvey 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 <yury.fridlyand@improving.com>

Use pagination in correctess tests too.

eda6280

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

spotless

9ba8341

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

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 <yury.fridlyand@improving.com>

acarbonetto reviewed Aug 18, 2023

View reviewed changes

Yury-Fridlyand merged commit 1bb815b into integ-ignore-pagination-switch Aug 18, 2023

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

Closed

6 tasks

Conversation

Yury-Fridlyand commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

codecov bot commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

matthewryanwells commented Aug 1, 2023

Uh oh!

Yury-Fridlyand commented Aug 1, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Yury-Fridlyand commented Aug 1, 2023 •

edited

Loading

codecov bot commented Aug 1, 2023 •

edited

Loading