Add integration JDBC tests for cursor/fetch_size feature. #208

Yury-Fridlyand · 2023-01-20T22:01:45Z

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

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.

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

`integDevJdbcTest`

Run ITs from **/devJdbc/*.java. Existing tests in CursorIT create and use DB connection using JDBC driver from source code or existing binary.

Optional parameters:

-Dtest.debug
Enabled test debug.
--tests '<test_filter>
Filter test using wildcard, could be specified multiple times.
-DjdbcFile=<file>
JDBC driver compiled binary (*.jar); if given, workflow uses it and ignores jdbcRepo and jdbcBranch parameters described below.
-DjdbcRepo=<repo_path>
JDBC driver repo to checkout and build from; uses https://github.com/opensearch-project/sql by default.
-DjdbcBranch=<branch>
Repository branch to pick code from; uses main by default.

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

Examples:

./gradlew :integ-test:integJdbcTest :integ-test:integDevJdbcTest

./gradlew :integ-test:integDevJdbcTest '-DjdbcFile=/mnt/c/GitHub/sql-jdbc/build/libs/opensearch-sql-jdbc-2.0.0.0.jar'

./gradlew :integ-test:integDevJdbcTest '-DjdbcRepo=https://github.com/Bit-Quill/sql-jdbc' '-DjdbcBranch=dev-feature-fix'

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

> Task :integ-test:integDevJdbcTest
org.opensearch.sql.devJdbc.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

opensearch-project-sql/integ-test/build.gradle

Lines 204 to 206 in 24de6ff

    
           filter { 
        
               includeTestsMatching '*.jdbc.*' 
        
           }

requires

opensearch-project-sql/integ-test/build.gradle

Line 177 in 24de6ff

useJUnitPlatform()

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.

MaxKsyunz

Thank you!

What's the difference between small table and big table?

Yury-Fridlyand · 2023-01-20T22:05:07Z

What's the difference between small table and big table?

calcs: 17 lines
bank: 7
account: 1k
online: 10k

integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java

GabeFernandez310 · 2023-01-26T23:48:01Z

integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java

+  @AfterClass
+  @SneakyThrows
+  public static void closeConnection() {
+    // TODO should we close Statement and ResultSet?


Was this answered or was this left on purpose for us to see?

It is not answered
I suppose we can leave things unclosed in tests.

integ-test/build.gradle

acarbonetto · 2023-01-27T19:22:38Z

integ-test/build.gradle

+    file("sql-jdbc").deleteDir()
+}
+
+task cloneJdbcDriverRepo {


Suggested change

task cloneJdbcDriverRepo {

task jdbcShadowJarFromRepo {

Or better yet, separate clone from build so that we don't need to delete/download the JDBC driver from repo each and every time.

It was your requirement to run tests on in-development JDBC driver. Note, this executed in :integ-test:integDevJdbcTest gradle task only if jdbcFile parameter not given. This gradle task is not a part of build or :integ-test:integTest tasks.

Yes. I just think those two tasks could be split into separate tasks rather than having to download + build... in case the build fails.

Mostly a suggestion I think at this point.

Fixed in 0b817f4

acarbonetto · 2023-01-27T19:23:45Z

integ-test/build.gradle

+
+        exec {
+            // clone the sql-jdbc repo locally
+            commandLine 'git', 'clone', System.getProperty('jdbcRepo', 'https://github.com/opensearch-project/sql-jdbc.git')


Can you also get a system property for sql-jdbc branch (System.getProperty("jdbcBranch", 'main'))?
That being said... don't default to main, you are assuming that main is the default branch in the repo, but that's defined by the repo and will download the default branch for you if you don't include --branch

git clone --branch <branchname> <remote-repo-url>

Fixed in 482da6b

acarbonetto · 2023-01-27T19:25:35Z

integ-test/build.gradle

+        }
+        exec {
+            workingDir 'sql-jdbc'
+            commandLine 'git', 'switch', System.getProperty("jdbcBranch", 'main')


remove to just clone the appropriate branch

Fixed in 482da6b

acarbonetto · 2023-01-27T19:30:03Z

integ-test/build.gradle

+    }
+
+    filter {
+        includeTestsMatching '*.devJdbc.*'


isn't this just one or two files...?

Only one file as of now, but I assume there would be more files in future.

I think we should specify a project, like org.opensearch.sql.devJdbc.*

Fixed in d847404

acarbonetto · 2023-01-27T19:31:27Z

integ-test/build.gradle

@@ -406,4 +528,6 @@ task integTestRemote(type: RestIntegTestTask) {
    exclude 'org/opensearch/sql/legacy/TermQueryExplainIT.class'
    exclude 'org/opensearch/sql/legacy/QueryAnalysisIT.class'
    exclude 'org/opensearch/sql/legacy/OrderIT.class'
+    exclude '**/jdbc/**'


I'd include the root directory structure... like

org/opensearch/sql/jdbc/** org/opensearch/sql/devJdbc/**

Fixed in 482da6b

acarbonetto · 2023-01-27T19:31:45Z

integ-test/build.gradle

@@ -214,6 +326,10 @@ task comparisonTest(type: RestIntegTestTask) {
    exclude 'org/opensearch/sql/ppl/**/*IT.class'
    exclude 'org/opensearch/sql/legacy/**/*IT.class'

+    // Exclude JDBC related tests
+    exclude '**/jdbc/**'


I'd include the root directory structure... like

org/opensearch/sql/jdbc/**
org/opensearch/sql/devJdbc/**

Fixed in 482da6b

acarbonetto · 2023-01-27T19:31:49Z

integ-test/build.gradle

@@ -192,10 +297,17 @@ integTest {

    // Skip this IT because all assertions are against explain output
    exclude 'org/opensearch/sql/legacy/OrderIT.class'
+
+    // Exclude JDBC related tests
+    exclude '**/jdbc/**'


I'd include the root directory structure... like

org/opensearch/sql/jdbc/**
org/opensearch/sql/devJdbc/**

Fixed in 482da6b

acarbonetto · 2023-01-27T19:34:28Z

integ-test/src/test/java/org/opensearch/sql/devJdbc/CursorIT.java

+  @BeforeEach
+  @SneakyThrows
+  public void init() {
+    if (!initialized) {


why isn't this under the @BeforeAll below?

I call initClient from init, which can't be called from a static method. @BeforeAll requires method to be static.

You could also do

if (client() == null) { initClient(); }

like the other classes.

initClient doesn't work in a static context

MaxKsyunz

@Yury-Fridlyand Could you split the integration tests from the build changes?

The integration tests look good, but support for a custom JDBC driver can be done in more standard way.

After applying the diffs below, you'll be able to use a custom JDBC driver by building it with ./gradlew publishToMavenLocal -Pversion=1.13.0-MAXSPECIAL
and then using 1.13.0-MAXSPECIAL in integration tests by running ./gradlew :integTest:build -Pjdbc_driver_version=1.99.0-MAXSPECIAL

This would also support snapshot builds of JDBC driver if OpenSearch project ever provides them.

The above requires this change to sqj-jdbc:

diff --git a/build.gradle b/build.gradle
index 436cad3..b91f7de 100644
--- a/build.gradle
+++ b/build.gradle
@@ -24,9 +24,6 @@ plugins {

 group 'org.opensearch.driver'

-// keep version in sync with version in Driver source
-version '1.2.0.0'
-
 boolean snapshot = "true".equals(System.getProperty("build.snapshot", "false"));
 if (snapshot) {
     version += "-SNAPSHOT"

and this change to sql:

diff --git a/gradle.properties b/gradle.properties
index 5cf428415..81338cd21 100644
--- a/gradle.properties
+++ b/gradle.properties
@@ -3,4 +3,5 @@
 # SPDX-License-Identifier: Apache-2.0

 version=1.13.0
+jdbc_driver_version=1.2.0.0
 org.gradle.jvmargs=-Duser.language=en -Duser.country=US
diff --git a/integ-test/build.gradle b/integ-test/build.gradle
index c948dc77c..fc04f66f8 100644
--- a/integ-test/build.gradle
+++ b/integ-test/build.gradle
@@ -80,7 +80,7 @@ dependencies {
     testImplementation group: 'org.opensearch.test', name: 'framework', version: "${opensearch_version}"
     testImplementation group: 'org.opensearch.client', name: 'opensearch-rest-high-level-client', version: "${opensearch_version}"
     testImplementation group: 'org.opensearch.client', name: 'opensearch-rest-client', version: "${opensearch_version}"
-    testImplementation group: 'org.opensearch.driver', name: 'opensearch-sql-jdbc', version: '1.2.0.0'
+    testImplementation group: 'org.opensearch.driver', name: 'opensearch-sql-jdbc', version: "${jdbc_driver_version}"
     testImplementation group: 'org.hamcrest', name: 'hamcrest', version: '2.1'
     implementation group: 'org.apache.logging.log4j', name: 'log4j-core', version:'2.17.1'
     testImplementation project(':opensearch-sql-plugin')

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

…pment JDBC driver. Signed-off-by: Yury-Fridlyand <[email protected]>

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

Yury-Fridlyand · 2023-01-31T21:25:49Z

Fixed in e9df987.
I slightly modified your idea. It works fine even without JDBC changes.
Prepend -DjdbcDriverVersion=<version> to :integ-test command line.

acarbonetto · 2023-01-31T22:19:06Z

integ-test/build.gradle

+    file("sql-jdbc").deleteDir()
+}
+
+task cloneJdbcDriverRepo {


Yes. I just think those two tasks could be split into separate tasks rather than having to download + build... in case the build fails.

acarbonetto · 2023-01-31T22:19:20Z

integ-test/build.gradle

+    file("sql-jdbc").deleteDir()
+}
+
+task cloneJdbcDriverRepo {


Mostly a suggestion I think at this point.

acarbonetto · 2023-01-31T22:20:03Z

integ-test/build.gradle

+    }
+
+    filter {
+        includeTestsMatching '*.devJdbc.*'


I think we should specify a project, like org.opensearch.sql.devJdbc.*

acarbonetto · 2023-01-31T22:25:41Z

integ-test/src/test/java/org/opensearch/sql/devJdbc/CursorIT.java

+  @BeforeEach
+  @SneakyThrows
+  public void init() {
+    if (!initialized) {


You could also do

if (client() == null) { initClient(); }

like the other classes.

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

* Add integration JDBC tests for cursor/fetch_size feature. Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand requested review from GumpacG, forestmvey, GabeFernandez310, MaxKsyunz, acarbonetto, margarit-h and matthewryanwells January 20, 2023 22:01

MaxKsyunz approved these changes Jan 20, 2023

View reviewed changes

This comment was marked as spam.

Sign in to view

margarit-h approved these changes Jan 20, 2023

View reviewed changes

GumpacG reviewed Jan 21, 2023

View reviewed changes

integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java Outdated Show resolved Hide resolved

Yury-Fridlyand requested review from margarit-h, MaxKsyunz and GumpacG January 24, 2023 01:52

GumpacG approved these changes Jan 24, 2023

View reviewed changes

GabeFernandez310 approved these changes Jan 26, 2023

View reviewed changes

acarbonetto requested changes Jan 30, 2023

View reviewed changes

MaxKsyunz requested changes Jan 31, 2023

View reviewed changes

Yury-Fridlyand added 4 commits January 31, 2023 13:22

Add integration JDBC tests for cursor/fetch_size feature.

bf18725

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

Rework tests and test suite. Add new test suite for testing in-develo…

bac1627

…pment JDBC driver. Signed-off-by: Yury-Fridlyand <[email protected]>

Address PR feedback.

4b47aac

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

Add option to specify JDBC maven artifact version from command line.

e9df987

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

Yury-Fridlyand force-pushed the dev-it-pagination-jdbc branch from 482da6b to e9df987 Compare January 31, 2023 21:22

Yury-Fridlyand requested review from acarbonetto and MaxKsyunz January 31, 2023 21:26

acarbonetto approved these changes Jan 31, 2023

View reviewed changes

Yury-Fridlyand added 2 commits January 31, 2023 15:16

Minor fix in the test filter.

d847404

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

Change workflow to keep JDBC repo, but refresh the branch. Update path.

0b817f4

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

forestmvey approved these changes Feb 2, 2023

View reviewed changes

Yury-Fridlyand merged commit 07fcd30 into integ-it-pagination-jdbc Feb 2, 2023

Yury-Fridlyand deleted the dev-it-pagination-jdbc branch February 2, 2023 17:47

Yury-Fridlyand mentioned this pull request Feb 2, 2023

Add integration JDBC tests for cursor/fetch_size feature opensearch-project/sql#1315

Merged

6 tasks

Yury-Fridlyand added a commit that referenced this pull request Mar 3, 2023

Add integration JDBC tests for cursor/fetch_size feature. (#208)

5754cab

* Add integration JDBC tests for cursor/fetch_size feature. Signed-off-by: Yury-Fridlyand <[email protected]>

Add integration JDBC tests for cursor/fetch_size feature. #208

Add integration JDBC tests for cursor/fetch_size feature. #208

Conversation

Yury-Fridlyand commented Jan 20, 2023 • edited Loading

Description

integJdbcTest

integDevJdbcTest

Notes

Issues Resolved

Check List

MaxKsyunz left a comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Jan 20, 2023

This comment was marked as spam.

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 Jan 30, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKsyunz left a comment • edited Loading

Choose a reason for hiding this comment

Yury-Fridlyand commented Jan 31, 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

Yury-Fridlyand commented Jan 20, 2023 •

edited

Loading

`integJdbcTest`

`integDevJdbcTest`

Yury-Fridlyand Jan 30, 2023 •

edited

Loading

MaxKsyunz left a comment •

edited

Loading