Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add integration JDBC tests for cursor/fetch_size feature #1315

Merged

Conversation

Yury-Fridlyand
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand commented Feb 2, 2023

  • Add integration JDBC tests for cursor/fetch_size feature.

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

See team review and discussion on Bit-Quill#208

Description

Add new IT suite which uses JDBC to issue queries with and without fetch_size set.
fetch_size parameter invokes pagination feature (cursor) which is currently supported by legacy engine only.

New :integ-test tasks:

integJdbcTest

Run ITs from **/jdbc/*.java. Existing tests in CursorIT create and use DB connection using JDBC driver from maven artifact.

Optional parameters:

  • -Dtest.debug
    Enabled test debug.
  • --tests '<test_filter>
    Filter test using wildcard, could be specified multiple times.
  • -DjdbcDriverVersion=<version>
    Maven artifact version for JDBC driver to use; optional, defaulted to 1.2.0.0.
  • -DjdbcFile=<file>
    Relative or absolute path to JDBC driver compiled binary (*.jar); if given, test platform uses it.
  • -DjdbcRepo=<repo_path>
    Relative or absolute path to JDBC driver Maven repo to get the driver from. Note: it is highly recommended to have non-default version of the driver in this repo to avoid conflicts.

Test cluster log located in integ-test/build/testclusters/integJdbcTest-0/logs/integJdbcTest.log
Test report saved in integ-test/build/reports/tests/integJdbcTest

Examples:

./gradlew :integ-test:integJdbcTest
# go to SQL JDBC repo
cd sql-jdbc
# build the driver
./gradlew build
# go to SQL plugin repo
cd ../sql
# run tests
./gradlew :integ-test:integJdbcTest '-DjdbcFile=../sql-jdbc/build/libs/opensearch-sql-jdbc-2.0.0.0.jar'
# go to SQL JDBC repo
cd sql-jdbc
# build the driver and publish to local repo
./gradlew publishShadowPublicationToLocalRepoRepository
# go to SQL plugin repo
cd ../sql
# run tests
./gradlew :integ-test:integJdbcTest '-DjdbcRepo=../sql-jdbc/build/repository/' -DjdbcDriverVersion=2.0.0.0
# run tests with non-default (e.g. non-latest) released JDBC version
./gradlew :integ-test:integJdbcTest -DjdbcDriverVersion=1.0.0.0

Output sample:

> Task :integ-test:integJdbcTest
org.opensearch.sql.jdbc.CursorIT.select_count_all_no_cursor(): SUCCESS 19.634s

CursorIT > select count all no cursor PASSED
org.opensearch.sql.jdbc.CursorIT.select_all_big_table_small_cursor(): SUCCESS 8.738s

CursorIT > select all big table small cursor PASSED
org.opensearch.sql.jdbc.CursorIT.select_all_small_table_small_cursor(): SUCCESS 0.238s

CursorIT > select all small table small cursor PASSED
org.opensearch.sql.jdbc.CursorIT.select_all_no_cursor(): SUCCESS 4.163s

CursorIT > select all no cursor PASSED
org.opensearch.sql.jdbc.CursorIT.select_all_small_table_big_cursor(): SUCCESS 0.132s

CursorIT > select all small table big cursor PASSED
org.opensearch.sql.jdbc.CursorIT.select_all_big_table_big_cursor(): SUCCESS 3.398s

CursorIT > select all big table big cursor PASSED
org.opensearch.sql.jdbc.CursorIT.dev_select_all_small_table_small_cursor(): SUCCESS 20.008s

CursorIT > dev_select_all_small_table_small_cursor() PASSED

Notes

Test filter in gradle
https://github.com/Bit-Quill/opensearch-project-sql/blob/24de6ff9eae35a824a664a865ad349788b744288/integ-test/build.gradle#L204-L206
requires
https://github.com/Bit-Quill/opensearch-project-sql/blob/24de6ff9eae35a824a664a865ad349788b744288/integ-test/build.gradle#L177
and doesn't work with JUnit 4. All IT framework uses Junit 4.
So, init() method of test classed isn't being called from superclasses automatically.
@BeforeAll/@BeforeClass require a static method, so I can't call initClient() there. I have to move initClient() to @BeforeEach with initialized flag.

New gradle tasks are not included into CI workflow.

Issues Resolved

N/A

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.

@Yury-Fridlyand Yury-Fridlyand marked this pull request as ready for review February 4, 2023 02:22
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner February 4, 2023 02:22
@dai-chen dai-chen added the infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label Feb 8, 2023
Copy link
Collaborator

@MaxKsyunz MaxKsyunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being able to set JDBC driver version is sufficient to run these tests against a development version of the driver.

I'd keep jdbc integration test suite and remove jdbcDev, cloneJdbcDriverRepo, and integDevJdbcTest .

Copy link
Collaborator

@MaxKsyunz MaxKsyunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the cloneJdbcDriverRepo task is breaking both SQL Java CI and CodeQL-Scan.

Yury-Fridlyand added a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Feb 14, 2023
…(review)

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2023

Codecov Report

Merging #1315 (891b18f) into main (ef38389) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##               main    #1315   +/-   ##
=========================================
  Coverage     98.38%   98.38%           
  Complexity     3698     3698           
=========================================
  Files           343      343           
  Lines          9121     9121           
  Branches        586      586           
=========================================
  Hits           8974     8974           
  Misses          142      142           
  Partials          5        5           
Flag Coverage Δ
sql-engine 98.38% <ø> (ø)

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

MaxKsyunz
MaxKsyunz previously approved these changes Feb 14, 2023
acarbonetto
acarbonetto previously approved these changes Feb 14, 2023
* Add integration JDBC tests for cursor/fetch_size feature.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
…(review)

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
This reverts commit dbc5b32.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand
Copy link
Collaborator Author

Rebased + added a fix for ITs. Shouldn't affect BWC.

@Yury-Fridlyand Yury-Fridlyand changed the title Add integration JDBC tests for cursor/fetch_size feature. (#208) Add integration JDBC tests for cursor/fetch_size feature Mar 10, 2023
…-jdbc

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

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this test!

@Yury-Fridlyand Yury-Fridlyand merged commit 57ccb6c into opensearch-project:main Apr 18, 2023
@Yury-Fridlyand Yury-Fridlyand deleted the integ-it-pagination-jdbc branch April 18, 2023 22:39
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 18, 2023
* Add integration JDBC tests for cursor/fetch_size feature.

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
(cherry picked from commit 57ccb6c)
acarbonetto pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Apr 28, 2023
…project#1315)

* Add integration JDBC tests for cursor/fetch_size feature.

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand Yury-Fridlyand added the pagination Pagination feature, ref #656 label May 1, 2023
Yury-Fridlyand added a commit that referenced this pull request May 8, 2023
* Add integration JDBC tests for cursor/fetch_size feature.

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
(cherry picked from commit 57ccb6c)

Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. pagination Pagination feature, ref #656 v2.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants