From c318dc5f3ca4b687f8acca664f18286b4b48e4bb Mon Sep 17 00:00:00 2001 From: Dai Date: Mon, 22 Jun 2020 12:53:43 -0700 Subject: [PATCH 01/12] Add comparison test base class and sample IT --- .../sql/sql/SQLIntegTestCase.java | 91 +++++++++++++++++++ .../sql/sql/SelectIT.java | 28 ++++++ 2 files changed, 119 insertions(+) create mode 100644 integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java create mode 100644 integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java new file mode 100644 index 0000000000..70c0edf6b9 --- /dev/null +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java @@ -0,0 +1,91 @@ +/* + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.sql; + +import com.amazon.opendistroforelasticsearch.sql.correctness.TestConfig; +import com.amazon.opendistroforelasticsearch.sql.correctness.report.TestReport; +import com.amazon.opendistroforelasticsearch.sql.correctness.report.TestSummary; +import com.amazon.opendistroforelasticsearch.sql.correctness.runner.ComparisonTest; +import com.amazon.opendistroforelasticsearch.sql.correctness.runner.connection.DBConnection; +import com.amazon.opendistroforelasticsearch.sql.correctness.runner.connection.ESConnection; +import com.amazon.opendistroforelasticsearch.sql.correctness.runner.connection.JDBCConnection; +import com.amazon.opendistroforelasticsearch.sql.correctness.testset.TestDataSet; +import com.amazon.opendistroforelasticsearch.sql.correctness.testset.TestQuerySet; +import com.amazon.opendistroforelasticsearch.sql.legacy.RestIntegTestCase; +import com.google.common.collect.Maps; +import java.util.Map; +import org.junit.Assert; + +/** + * SQL integration test base class. + */ +public abstract class SQLIntegTestCase extends RestIntegTestCase { + + private static ComparisonTest runner; + + @Override + protected void init() throws Exception { + TestConfig config = new TestConfig(getCmdLineArgs()); + runner = new ComparisonTest(getThisDBConnection(config), + getOtherDBConnections(config)); + + runner.connect(); + for (TestDataSet dataSet : config.getTestDataSets()) { + runner.loadData(dataSet); + } + } + + /** + * Execute the given query and compare result with other database. + */ + protected void verify(String query) { + TestReport result = runner.verify(new TestQuerySet(query)); + TestSummary summary = result.getSummary(); + Assert.assertEquals("Comparison failed: " + result, 0, summary.getFailure()); + } + + private Map getCmdLineArgs() { + return Maps.fromProperties(System.getProperties()); + } + + private DBConnection getThisDBConnection(TestConfig config) { + String dbUrl = config.getDbConnectionUrl(); + if (dbUrl.isEmpty()) { + return getESConnection(config); + } + return new JDBCConnection("DB Tested", dbUrl); + } + + /** + * Use Elasticsearch cluster initialized by ES Gradle task. + */ + private DBConnection getESConnection(TestConfig config) { + String esHost = client().getNodes().get(0).getHost().toString(); + return new ESConnection("jdbc:elasticsearch://" + esHost, client()); + } + + /** + * Create database connection with database name and connect URL. + */ + private DBConnection[] getOtherDBConnections(TestConfig config) { + return config.getOtherDbConnectionNameAndUrls() + .entrySet().stream() + .map(e -> new JDBCConnection(e.getKey(), e.getValue())) + .toArray(DBConnection[]::new); + } + +} diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java new file mode 100644 index 0000000000..106b94a5b1 --- /dev/null +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java @@ -0,0 +1,28 @@ +/* + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.sql; + +import org.junit.Test; + +public class SelectIT extends SQLIntegTestCase { + + @Test + public void select() { + verify("SELECT DistanceMiles FROM kibana_sample_data_flights"); + } + +} From 5023d4cf0616879e6ce12ad830a3b885221037d5 Mon Sep 17 00:00:00 2001 From: Dai Date: Mon, 22 Jun 2020 14:47:36 -0700 Subject: [PATCH 02/12] Close connection --- integ-test/build.gradle | 2 +- .../runner/connection/ESConnection.java | 4 +- .../sql/sql/SQLIntegTestCase.java | 38 +++++++++++++++++-- .../sql/sql/SelectIT.java | 7 ++++ 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/integ-test/build.gradle b/integ-test/build.gradle index 4b674b16ce..dae099c3ea 100644 --- a/integ-test/build.gradle +++ b/integ-test/build.gradle @@ -31,7 +31,7 @@ dependencies { testRuntimeOnly('org.junit.jupiter:junit-jupiter-engine:5.6.2') // JDBC drivers for comparison test. Somehow Apache Derby throws security permission exception. - testCompile group: 'com.amazon.opendistroforelasticsearch.client', name: 'opendistro-sql-jdbc', version: '1.3.0.0' + testCompile group: 'com.amazon.opendistroforelasticsearch.client', name: 'opendistro-sql-jdbc', version: '1.8.0.0' testCompile group: 'com.h2database', name: 'h2', version: '1.4.200' testCompile group: 'org.xerial', name: 'sqlite-jdbc', version: '3.28.0' //testCompile group: 'org.apache.derby', name: 'derby', version: '10.15.1.3' diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java index fc4b6a0236..9f131d41d9 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java @@ -80,11 +80,11 @@ public DBResult select(String query) { @Override public void close() { connection.close(); - try { + /*try { client.close(); } catch (IOException e) { // Ignore - } + }*/ } private void performRequest(Request request) { diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java index 70c0edf6b9..6f2a7fb524 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java @@ -26,19 +26,30 @@ import com.amazon.opendistroforelasticsearch.sql.correctness.testset.TestDataSet; import com.amazon.opendistroforelasticsearch.sql.correctness.testset.TestQuerySet; import com.amazon.opendistroforelasticsearch.sql.legacy.RestIntegTestCase; +import com.amazon.opendistroforelasticsearch.sql.legacy.utils.StringUtils; +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; import com.google.common.collect.Maps; import java.util.Map; +import org.junit.AfterClass; import org.junit.Assert; /** * SQL integration test base class. */ +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) public abstract class SQLIntegTestCase extends RestIntegTestCase { + /** + * Comparison test runner shared by all methods in this IT class. + */ private static ComparisonTest runner; @Override protected void init() throws Exception { + if (runner != null) { + return; + } + TestConfig config = new TestConfig(getCmdLineArgs()); runner = new ComparisonTest(getThisDBConnection(config), getOtherDBConnections(config)); @@ -49,23 +60,42 @@ protected void init() throws Exception { } } + /** + * Clean up test data and close other database connection. + */ + @AfterClass + public static void cleanUp() { + + /*TestConfig config = new TestConfig(getCmdLineArgs()); + for (TestDataSet dataSet : config.getTestDataSets()) { + runner.cleanUp(dataSet); + }*/ + + try { + runner.close(); + } finally { + runner = null; + } + } + /** * Execute the given query and compare result with other database. */ protected void verify(String query) { TestReport result = runner.verify(new TestQuerySet(query)); TestSummary summary = result.getSummary(); - Assert.assertEquals("Comparison failed: " + result, 0, summary.getFailure()); + Assert.assertEquals(StringUtils.format( + "Comparison test failed on query [%s]: %s", query, result), 0, summary.getFailure()); } - private Map getCmdLineArgs() { + private static Map getCmdLineArgs() { return Maps.fromProperties(System.getProperties()); } private DBConnection getThisDBConnection(TestConfig config) { String dbUrl = config.getDbConnectionUrl(); if (dbUrl.isEmpty()) { - return getESConnection(config); + return getESConnection(); } return new JDBCConnection("DB Tested", dbUrl); } @@ -73,7 +103,7 @@ private DBConnection getThisDBConnection(TestConfig config) { /** * Use Elasticsearch cluster initialized by ES Gradle task. */ - private DBConnection getESConnection(TestConfig config) { + private DBConnection getESConnection() { String esHost = client().getNodes().get(0).getHost().toString(); return new ESConnection("jdbc:elasticsearch://" + esHost, client()); } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java index 106b94a5b1..e95507b5dc 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java @@ -25,4 +25,11 @@ public void select() { verify("SELECT DistanceMiles FROM kibana_sample_data_flights"); } + @Test + public void select2() { + verify( + "SELECT AvgTicketPrice, Carrier FROM kibana_sample_data_flights WHERE AvgTicketPrice <= 500" + ); + } + } From 099fd2a25e390d69ac7b26b4d8f7e228ec7ebea7 Mon Sep 17 00:00:00 2001 From: Dai Date: Mon, 22 Jun 2020 15:12:10 -0700 Subject: [PATCH 03/12] Add refresh=true --- .../sql/correctness/runner/connection/ESConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java index 9f131d41d9..150e2c2ccb 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java @@ -67,7 +67,7 @@ public void drop(String tableName) { @Override public void insert(String tableName, String[] columnNames, List batch) { - Request request = new Request("POST", "/" + tableName + "/_bulk"); + Request request = new Request("POST", "/" + tableName + "/_bulk?refresh=true"); request.setJsonEntity(buildBulkBody(columnNames, batch)); performRequest(request); } From 9957c3ba10e3f0d30e31568060d5196e28bfb0fb Mon Sep 17 00:00:00 2001 From: Dai Date: Mon, 22 Jun 2020 15:22:10 -0700 Subject: [PATCH 04/12] Add refresh=true --- .../sql/correctness/tests/ESConnectionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/ESConnectionTest.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/ESConnectionTest.java index 0f2b360aa4..16a24375ff 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/ESConnectionTest.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/ESConnectionTest.java @@ -77,7 +77,7 @@ public void testInsertData() throws IOException { Request actual = captureActualArg(); assertEquals("POST", actual.getMethod()); - assertEquals("/test/_bulk", actual.getEndpoint()); + assertEquals("/test/_bulk?refresh=true", actual.getEndpoint()); assertEquals( "{\"index\":{}}\n" + "{\"name\":\"John\"}\n" From 6ac52fd0572b9294060eefe0a441a6b60ec171de Mon Sep 17 00:00:00 2001 From: Dai Date: Mon, 22 Jun 2020 16:27:29 -0700 Subject: [PATCH 05/12] Add comment to explain why not close REST client --- .../sql/correctness/runner/connection/ESConnection.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java index 150e2c2ccb..8091d9e7e0 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java @@ -79,12 +79,9 @@ public DBResult select(String query) { @Override public void close() { + // Only close database connection and leave ES REST connection alone + // because it's initialized and manged by outside. connection.close(); - /*try { - client.close(); - } catch (IOException e) { - // Ignore - }*/ } private void performRequest(Request request) { From f13832301847101a443fe8d40df33b336318fbe0 Mon Sep 17 00:00:00 2001 From: Dai Date: Wed, 24 Jun 2020 18:11:03 -0700 Subject: [PATCH 06/12] Organize test case in txt file --- .../sql/sql/SQLCorrectnessIT.java | 56 +++++++++++++++++++ .../sql/sql/SQLIntegTestCase.java | 21 +++++-- .../sql/sql/SelectIT.java | 35 ------------ .../resources/correctness/bugfixes/521.txt | 1 + .../correctness/expressions/arithmetics.txt | 0 .../correctness/expressions/functions.txt | 0 .../correctness/expressions/literals.txt | 4 ++ .../correctness/expressions/predicates.txt | 0 .../resources/correctness/queries/from.txt | 0 .../resources/correctness/queries/groupby.txt | 0 .../resources/correctness/queries/joins.txt | 0 .../resources/correctness/queries/select.txt | 1 + .../correctness/queries/subquries.txt | 0 .../resources/correctness/queries/where.txt | 1 + 14 files changed, 78 insertions(+), 41 deletions(-) create mode 100644 integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java delete mode 100644 integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java create mode 100644 integ-test/src/test/resources/correctness/bugfixes/521.txt create mode 100644 integ-test/src/test/resources/correctness/expressions/arithmetics.txt create mode 100644 integ-test/src/test/resources/correctness/expressions/functions.txt create mode 100644 integ-test/src/test/resources/correctness/expressions/literals.txt create mode 100644 integ-test/src/test/resources/correctness/expressions/predicates.txt create mode 100644 integ-test/src/test/resources/correctness/queries/from.txt create mode 100644 integ-test/src/test/resources/correctness/queries/groupby.txt create mode 100644 integ-test/src/test/resources/correctness/queries/joins.txt create mode 100644 integ-test/src/test/resources/correctness/queries/select.txt create mode 100644 integ-test/src/test/resources/correctness/queries/subquries.txt create mode 100644 integ-test/src/test/resources/correctness/queries/where.txt diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java new file mode 100644 index 0000000000..77f206202c --- /dev/null +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java @@ -0,0 +1,56 @@ +/* + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + * + */ + +package com.amazon.opendistroforelasticsearch.sql.sql; + +import com.google.common.io.Resources; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.function.Consumer; +import org.junit.Test; + +/** + * SQL integration test automated by comparison test framework. + */ +public class SQLCorrectnessIT extends SQLIntegTestCase { + + private static final String ROOT_DIR = "correctness/"; + private static final String[] EXPR_TEST_DIR = { "expressions" }; + private static final String[] QUERY_TEST_DIR = { "queries"/*, "bugfixes"*/ }; + + @Test + public void runAllTests() throws Exception { + iterateAllFiles(EXPR_TEST_DIR, expr -> verify("SELECT " + expr)); + iterateAllFiles(QUERY_TEST_DIR, this::verify); + } + + @SuppressWarnings("UnstableApiUsage") + private void iterateAllFiles(String[] dirs, Consumer verify) throws Exception { + for (String dir : dirs) { + Path dirPath = Paths.get(Resources.getResource(ROOT_DIR + dir).toURI()); + Files.walk(dirPath).filter(Files::isRegularFile).forEach(file -> { + try { + Files.lines(file).forEach(verify); + } catch (IOException e) { + throw new IllegalStateException("Failed to read file: " + file, e); + } + }); + } + } + +} diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java index 6f2a7fb524..350462c57d 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java @@ -65,19 +65,28 @@ protected void init() throws Exception { */ @AfterClass public static void cleanUp() { - - /*TestConfig config = new TestConfig(getCmdLineArgs()); - for (TestDataSet dataSet : config.getTestDataSets()) { - runner.cleanUp(dataSet); - }*/ - try { + TestConfig config = new TestConfig(getCmdLineArgs()); + for (TestDataSet dataSet : config.getTestDataSets()) { + runner.cleanUp(dataSet); + } + runner.close(); } finally { runner = null; } } + /** + * Execute the given query and compare result with other database. + */ + protected void verify(String... queries) { + TestReport result = runner.verify(new TestQuerySet(queries)); + TestSummary summary = result.getSummary(); + Assert.assertEquals(StringUtils.format( + "Comparison test failed on queries: %s", result), 0, summary.getFailure()); + } + /** * Execute the given query and compare result with other database. */ diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java deleted file mode 100644 index e95507b5dc..0000000000 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SelectIT.java +++ /dev/null @@ -1,35 +0,0 @@ -/* - * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"). - * You may not use this file except in compliance with the License. - * A copy of the License is located at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * or in the "license" file accompanying this file. This file is distributed - * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either - * express or implied. See the License for the specific language governing - * permissions and limitations under the License. - * - */ - -package com.amazon.opendistroforelasticsearch.sql.sql; - -import org.junit.Test; - -public class SelectIT extends SQLIntegTestCase { - - @Test - public void select() { - verify("SELECT DistanceMiles FROM kibana_sample_data_flights"); - } - - @Test - public void select2() { - verify( - "SELECT AvgTicketPrice, Carrier FROM kibana_sample_data_flights WHERE AvgTicketPrice <= 500" - ); - } - -} diff --git a/integ-test/src/test/resources/correctness/bugfixes/521.txt b/integ-test/src/test/resources/correctness/bugfixes/521.txt new file mode 100644 index 0000000000..72a27b01d7 --- /dev/null +++ b/integ-test/src/test/resources/correctness/bugfixes/521.txt @@ -0,0 +1 @@ +SELECT timestamp, COUNT(*) FROM kibana_sample_data_flights GROUP BY timestamp \ No newline at end of file diff --git a/integ-test/src/test/resources/correctness/expressions/arithmetics.txt b/integ-test/src/test/resources/correctness/expressions/arithmetics.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integ-test/src/test/resources/correctness/expressions/functions.txt b/integ-test/src/test/resources/correctness/expressions/functions.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integ-test/src/test/resources/correctness/expressions/literals.txt b/integ-test/src/test/resources/correctness/expressions/literals.txt new file mode 100644 index 0000000000..74710d917b --- /dev/null +++ b/integ-test/src/test/resources/correctness/expressions/literals.txt @@ -0,0 +1,4 @@ +1 +true +-4.567 +-123,false diff --git a/integ-test/src/test/resources/correctness/expressions/predicates.txt b/integ-test/src/test/resources/correctness/expressions/predicates.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integ-test/src/test/resources/correctness/queries/from.txt b/integ-test/src/test/resources/correctness/queries/from.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integ-test/src/test/resources/correctness/queries/groupby.txt b/integ-test/src/test/resources/correctness/queries/groupby.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integ-test/src/test/resources/correctness/queries/joins.txt b/integ-test/src/test/resources/correctness/queries/joins.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integ-test/src/test/resources/correctness/queries/select.txt b/integ-test/src/test/resources/correctness/queries/select.txt new file mode 100644 index 0000000000..3c7bf576fc --- /dev/null +++ b/integ-test/src/test/resources/correctness/queries/select.txt @@ -0,0 +1 @@ +SELECT DistanceMiles FROM kibana_sample_data_flights \ No newline at end of file diff --git a/integ-test/src/test/resources/correctness/queries/subquries.txt b/integ-test/src/test/resources/correctness/queries/subquries.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/integ-test/src/test/resources/correctness/queries/where.txt b/integ-test/src/test/resources/correctness/queries/where.txt new file mode 100644 index 0000000000..9fce272bfa --- /dev/null +++ b/integ-test/src/test/resources/correctness/queries/where.txt @@ -0,0 +1 @@ +SELECT AvgTicketPrice, Carrier FROM kibana_sample_data_flights WHERE AvgTicketPrice <= 500 \ No newline at end of file From 6a684062f357057e7176190eb5504314b703aa5e Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 25 Jun 2020 09:48:17 -0700 Subject: [PATCH 07/12] Support fetch size 0 which is default request by JDBC driver From 900b465f5ed1ac570252036cbd4468aff9c5ae37 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 25 Jun 2020 12:58:15 -0700 Subject: [PATCH 08/12] Explain comparison test result for easy troubleshooting --- .../correctness/report/FailedTestCase.java | 9 +++ .../runner/resultset/DBResult.java | 67 +++++++++++++++++++ .../sql/correctness/runner/resultset/Row.java | 31 ++++++++- .../sql/correctness/tests/DBResultTest.java | 37 ++++++++++ .../sql/correctness/tests/RowTest.java | 11 +++ .../sql/correctness/tests/TestReportTest.java | 1 + .../sql/sql/SQLIntegTestCase.java | 9 ++- 7 files changed, 159 insertions(+), 6 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java index 6d6176c600..0aa0b56389 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java @@ -20,6 +20,7 @@ import com.amazon.opendistroforelasticsearch.sql.correctness.runner.resultset.DBResult; import java.util.Comparator; import java.util.List; +import java.util.stream.Collectors; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.ToString; @@ -37,10 +38,18 @@ public class FailedTestCase extends TestCaseReport { */ private final List resultSets; + private final String explain; + public FailedTestCase(int id, String sql, List resultSets) { super(id, sql, FAILURE); this.resultSets = resultSets; this.resultSets.sort(Comparator.comparing(DBResult::getDatabaseName)); + + // Generate explanation by diff the first result with remaining + this.explain = resultSets.subList(1, resultSets.size()) + .stream() + .map(result -> resultSets.get(0).diff(result)) + .collect(Collectors.joining(",")); } } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java index 5829997fb4..1bc63c2250 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java @@ -19,7 +19,10 @@ import com.google.common.collect.ImmutableSet; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; +import java.util.Iterator; +import java.util.List; import java.util.Set; import java.util.stream.Collectors; import lombok.EqualsAndHashCode; @@ -101,4 +104,68 @@ public Collection> getDataRows() { return dataRows.stream().map(Row::getValues).collect(Collectors.toSet()); } + /** + * Explain the difference between this and other DB result which is helpful for + * troubleshooting in final test report. + * @param other other DB result + * @return explain the difference + */ + public String diff(DBResult other) { + String result = diffSchema(other); + if (result.isEmpty()) { + return diffDataRows(other); + } + return result; + } + + private String diffSchema(DBResult other) { + if (schema.size() != other.schema.size()) { + return StringUtils.format( + "Schema size is different: this=[%d], other=[%d]", schema.size(), other.schema.size()); + } + + Iterator thisIt = schema.iterator(); + Iterator otherIt = other.schema.iterator(); + int i = 0; + while (thisIt.hasNext()) { + Type thisType = thisIt.next(); + Type otherType = otherIt.next(); + if (!thisType.equals(otherType)) { + return StringUtils.format( + "Schema type at [%d] is different: thisType=[%s], otherType=[%s]", + i, thisType, otherType); + } + i++; + } + return ""; + } + + private String diffDataRows(DBResult other) { + List thisRows = sort(dataRows); + List otherRows = sort(other.dataRows); + int thisSize = thisRows.size(); + int otherSize = otherRows.size(); + + if (thisSize != otherSize) { + return StringUtils.format( + "Data rows size is different: this=[%d], other=[%d]", thisSize, otherSize); + } + + for (int i = 0; i < thisSize; i++) { + Row thisRow = thisRows.get(i); + Row otherRow = otherRows.get(i); + if (!thisRow.equals(otherRow)) { + return StringUtils.format( + "Data row at [%d] is different: this=[%s], other=[%s]", i, thisRow, otherRow); + } + } + return ""; + } + + private static > List sort(Collection collection) { + ArrayList list = new ArrayList<>(collection); + Collections.sort(list); + return list; + } + } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/Row.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/Row.java index 85310d7bbd..d409bd81f2 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/Row.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/Row.java @@ -19,6 +19,7 @@ import java.math.RoundingMode; import java.util.ArrayList; import java.util.Collection; +import java.util.List; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.ToString; @@ -29,7 +30,7 @@ @EqualsAndHashCode @ToString @Getter -public class Row { +public class Row implements Comparable { private final Collection values; @@ -56,4 +57,32 @@ private Object roundFloatNum(Object value) { return value; } + @SuppressWarnings("unchecked") + @Override + public int compareTo(Row other) { + List thisObjects = new ArrayList<>(values); + List otherObjects = new ArrayList<>(other.values); + + for (int i = 0; i < thisObjects.size(); i++) { + Object thisObject = thisObjects.get(i); + Object otherObject = otherObjects.get(i); + + /* + * Only one is null, otherwise (both null or non-null) go ahead. + * Always consider NULL is greater which means NULL comes last in ASC and first in DESC + */ + if (thisObject == null ^ otherObject == null) { + return thisObject == null ? 1 : -1; + } + + if (thisObject instanceof Comparable) { + int result = ((Comparable) thisObject).compareTo(otherObject); + if (result != 0) { + return result; + } + } // Ignore incomparable field silently? + } + return 0; + } + } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java index cc1caed547..435a82af48 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java @@ -20,8 +20,12 @@ import static org.junit.Assert.assertNotEquals; import com.amazon.opendistroforelasticsearch.sql.correctness.runner.resultset.DBResult; +import com.amazon.opendistroforelasticsearch.sql.correctness.runner.resultset.Row; import com.amazon.opendistroforelasticsearch.sql.correctness.runner.resultset.Type; +import com.google.common.collect.Lists; +import com.google.common.collect.Sets; import java.util.Arrays; +import java.util.HashSet; import org.junit.Test; /** @@ -53,4 +57,37 @@ public void dbResultWithDifferentColumnTypeShouldNotEqual() { assertNotEquals(result1, result2); } + @Test + public void canExplainColumnTypeDifference() { + DBResult result1 = new DBResult("DB 1", + Arrays.asList(new Type("name", "VARCHAR"), new Type("age", "FLOAT")), emptyList()); + DBResult result2 = new DBResult("DB 2", + Arrays.asList(new Type("name", "VARCHAR"), new Type("age", "INT")), emptyList()); + + assertEquals( + "Schema type at [1] is different: " + + "thisType=[Type(name=age, type=FLOAT)], otherType=[Type(name=age, type=INT)]", + result1.diff(result2) + ); + } + + @Test + public void canExplainDataRowsDifference() { + DBResult result1 = new DBResult("DB 1", Arrays.asList(new Type("name", "VARCHAR")), + Sets.newHashSet( + new Row(Arrays.asList("hello")), + new Row(Arrays.asList("world")), + new Row(Lists.newArrayList((Object) null)))); + DBResult result2 = new DBResult("DB 2",Arrays.asList(new Type("name", "VARCHAR")), + Sets.newHashSet( + new Row(Lists.newArrayList((Object) null)), + new Row(Arrays.asList("hello")), + new Row(Arrays.asList("world123")))); + + assertEquals( + "Data row at [1] is different: this=[Row(values=[world])], other=[Row(values=[world123])]", + result1.diff(result2) + ); + } + } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/RowTest.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/RowTest.java index 5d43f6d5b5..28dbb480f8 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/RowTest.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/RowTest.java @@ -46,4 +46,15 @@ public void rowShouldNotEqualToOtherRowWithDifferentString() { assertNotEquals(row2, row1); } + @Test + public void shouldConsiderNullGreater() { + Row row1 = new Row(); + Row row2 = new Row(); + row1.add("hello"); + row1.add(null); + row2.add("hello"); + row2.add("world"); + assertEquals(1, row1.compareTo(row2)); + } + } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/TestReportTest.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/TestReportTest.java index b6cdfa76e3..fa77a98a2b 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/TestReportTest.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/TestReportTest.java @@ -83,6 +83,7 @@ public void testFailedReport() { " \"id\": 1," + " \"result\": 'Failed'," + " \"sql\": \"SELECT * FROM accounts\"," + + " \"explain\": \"Data row at [0] is different: this=[Row(values=[hello])], other=[Row(values=[world])]\"," + " \"resultSets\": [" + " {" + " \"database\": \"Elasticsearch\"," + diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java index 350462c57d..d194a66acf 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java @@ -30,6 +30,7 @@ import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; import com.google.common.collect.Maps; import java.util.Map; +import org.json.JSONObject; import org.junit.AfterClass; import org.junit.Assert; @@ -84,17 +85,15 @@ protected void verify(String... queries) { TestReport result = runner.verify(new TestQuerySet(queries)); TestSummary summary = result.getSummary(); Assert.assertEquals(StringUtils.format( - "Comparison test failed on queries: %s", result), 0, summary.getFailure()); + "Comparison test failed on queries: %s", new JSONObject(result).toString(2)), + 0, summary.getFailure()); } /** * Execute the given query and compare result with other database. */ protected void verify(String query) { - TestReport result = runner.verify(new TestQuerySet(query)); - TestSummary summary = result.getSummary(); - Assert.assertEquals(StringUtils.format( - "Comparison test failed on query [%s]: %s", query, result), 0, summary.getFailure()); + verify(new String[]{ query }); } private static Map getCmdLineArgs() { From 7f1283498209028defb94a0c851f6929ae9932c0 Mon Sep 17 00:00:00 2001 From: Dai Date: Thu, 25 Jun 2020 13:18:48 -0700 Subject: [PATCH 09/12] Run queries in a file by a batch --- .../sql/sql/SQLCorrectnessIT.java | 33 ++++++++++++------- .../sql/sql/SQLIntegTestCase.java | 5 +-- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java index 77f206202c..171da78f63 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java @@ -21,7 +21,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.function.Consumer; +import java.util.function.Function; import org.junit.Test; /** @@ -35,21 +35,32 @@ public class SQLCorrectnessIT extends SQLIntegTestCase { @Test public void runAllTests() throws Exception { - iterateAllFiles(EXPR_TEST_DIR, expr -> verify("SELECT " + expr)); - iterateAllFiles(QUERY_TEST_DIR, this::verify); + verifyQueries(EXPR_TEST_DIR, expr -> "SELECT " + expr); + verifyQueries(QUERY_TEST_DIR, Function.identity()); } + /** + * Verify queries in files in directories with a converter to preprocess query. + * For example, for expressions it is converted to a SELECT clause before testing. + */ @SuppressWarnings("UnstableApiUsage") - private void iterateAllFiles(String[] dirs, Consumer verify) throws Exception { + private void verifyQueries(String[] dirs, Function converter) throws Exception { for (String dir : dirs) { Path dirPath = Paths.get(Resources.getResource(ROOT_DIR + dir).toURI()); - Files.walk(dirPath).filter(Files::isRegularFile).forEach(file -> { - try { - Files.lines(file).forEach(verify); - } catch (IOException e) { - throw new IllegalStateException("Failed to read file: " + file, e); - } - }); + Files.walk(dirPath) + .filter(Files::isRegularFile) + .forEach(file -> verifyQueries(file, converter)); + } + } + + private void verifyQueries(Path file, Function converter) { + try { + String[] queries = Files.lines(file) + .map(converter) + .toArray(String[]::new); + verify(queries); + } catch (IOException e) { + throw new IllegalStateException("Failed to read file: " + file, e); } } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java index d194a66acf..b3c19f7a1f 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java @@ -79,7 +79,8 @@ public static void cleanUp() { } /** - * Execute the given query and compare result with other database. + * Execute the given queries and compare result with other database. + * The queries will be considered as one test batch. */ protected void verify(String... queries) { TestReport result = runner.verify(new TestQuerySet(queries)); @@ -90,7 +91,7 @@ protected void verify(String... queries) { } /** - * Execute the given query and compare result with other database. + * Execute a single given query and compare result with other database. */ protected void verify(String query) { verify(new String[]{ query }); From 51de1d558979c202539cfb445d1825c5672b0c5e Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 26 Jun 2020 08:07:17 -0700 Subject: [PATCH 10/12] Prepare for PR --- .../correctness/report/FailedTestCase.java | 3 + .../runner/resultset/DBResult.java | 55 +++++++++++-------- .../sql/correctness/tests/DBResultTest.java | 5 +- .../sql/sql/SQLCorrectnessIT.java | 2 +- .../sql/sql/SQLIntegTestCase.java | 32 +++-------- 5 files changed, 44 insertions(+), 53 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java index 0aa0b56389..c381ec6220 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java @@ -38,6 +38,9 @@ public class FailedTestCase extends TestCaseReport { */ private final List resultSets; + /** + * Explain where the difference is caused the test failure. + */ private final String explain; public FailedTestCase(int id, String sql, List resultSets) { diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java index 1bc63c2250..ccf9a24187 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java @@ -21,7 +21,6 @@ import java.util.Collection; import java.util.Collections; import java.util.HashSet; -import java.util.Iterator; import java.util.List; import java.util.Set; import java.util.stream.Collectors; @@ -124,18 +123,13 @@ private String diffSchema(DBResult other) { "Schema size is different: this=[%d], other=[%d]", schema.size(), other.schema.size()); } - Iterator thisIt = schema.iterator(); - Iterator otherIt = other.schema.iterator(); - int i = 0; - while (thisIt.hasNext()) { - Type thisType = thisIt.next(); - Type otherType = otherIt.next(); - if (!thisType.equals(otherType)) { - return StringUtils.format( - "Schema type at [%d] is different: thisType=[%s], otherType=[%s]", - i, thisType, otherType); - } - i++; + List thisTypes = new ArrayList<>(schema); + List otherTypes = new ArrayList<>(other.schema); + int diff = findFirstDifference(thisTypes, otherTypes); + if (diff >= 0) { + return StringUtils.format( + "Schema type at [%d] is different: thisType=[%s], otherType=[%s]", + diff, thisTypes.get(diff), otherTypes.get(diff)); } return ""; } @@ -143,25 +137,38 @@ private String diffSchema(DBResult other) { private String diffDataRows(DBResult other) { List thisRows = sort(dataRows); List otherRows = sort(other.dataRows); - int thisSize = thisRows.size(); - int otherSize = otherRows.size(); - if (thisSize != otherSize) { + if (thisRows.size() != otherRows.size()) { return StringUtils.format( - "Data rows size is different: this=[%d], other=[%d]", thisSize, otherSize); + "Data rows size is different: this=[%d], other=[%d]", + thisRows.size(), otherRows.size()); } - for (int i = 0; i < thisSize; i++) { - Row thisRow = thisRows.get(i); - Row otherRow = otherRows.get(i); - if (!thisRow.equals(otherRow)) { - return StringUtils.format( - "Data row at [%d] is different: this=[%s], other=[%s]", i, thisRow, otherRow); - } + int diff = findFirstDifference(thisRows, otherRows); + if (diff >= 0) { + return StringUtils.format( + "Data row at [%d] is different: this=[%s], other=[%s]", + diff, thisRows.get(diff), otherRows.get(diff)); } return ""; } + /** + * Find first different element with assumption that the lists given have same size + * and there is no NULL element inside. + */ + private static int findFirstDifference(List list1, List list2) { + for (int i = 0; i < list1.size(); i++) { + if (!list1.get(i).equals(list2.get(i))) { + return i; + } + } + return -1; + } + + /** + * Convert a collection to list and sort and return this new list. + */ private static > List sort(Collection collection) { ArrayList list = new ArrayList<>(collection); Collections.sort(list); diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java index 435a82af48..76f2d13178 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java @@ -25,7 +25,6 @@ import com.google.common.collect.Lists; import com.google.common.collect.Sets; import java.util.Arrays; -import java.util.HashSet; import org.junit.Test; /** @@ -58,7 +57,7 @@ public void dbResultWithDifferentColumnTypeShouldNotEqual() { } @Test - public void canExplainColumnTypeDifference() { + public void shouldExplainColumnTypeDifference() { DBResult result1 = new DBResult("DB 1", Arrays.asList(new Type("name", "VARCHAR"), new Type("age", "FLOAT")), emptyList()); DBResult result2 = new DBResult("DB 2", @@ -72,7 +71,7 @@ public void canExplainColumnTypeDifference() { } @Test - public void canExplainDataRowsDifference() { + public void shouldExplainDataRowsDifference() { DBResult result1 = new DBResult("DB 1", Arrays.asList(new Type("name", "VARCHAR")), Sets.newHashSet( new Row(Arrays.asList("hello")), diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java index 171da78f63..c052d73cda 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java @@ -31,7 +31,7 @@ public class SQLCorrectnessIT extends SQLIntegTestCase { private static final String ROOT_DIR = "correctness/"; private static final String[] EXPR_TEST_DIR = { "expressions" }; - private static final String[] QUERY_TEST_DIR = { "queries"/*, "bugfixes"*/ }; + private static final String[] QUERY_TEST_DIR = { "queries"/*, "bugfixes"*/ }; //TODO: skip bugfixes folder for now since it fails @Test public void runAllTests() throws Exception { diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java index b3c19f7a1f..faef8f713f 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLIntegTestCase.java @@ -16,6 +16,8 @@ package com.amazon.opendistroforelasticsearch.sql.sql; +import static java.util.Collections.emptyMap; + import com.amazon.opendistroforelasticsearch.sql.correctness.TestConfig; import com.amazon.opendistroforelasticsearch.sql.correctness.report.TestReport; import com.amazon.opendistroforelasticsearch.sql.correctness.report.TestSummary; @@ -28,14 +30,13 @@ import com.amazon.opendistroforelasticsearch.sql.legacy.RestIntegTestCase; import com.amazon.opendistroforelasticsearch.sql.legacy.utils.StringUtils; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; -import com.google.common.collect.Maps; -import java.util.Map; import org.json.JSONObject; import org.junit.AfterClass; import org.junit.Assert; /** - * SQL integration test base class. + * SQL integration test base class. This is very similar to CorrectnessIT though + * enforce the success of all tests rather than report failures only. */ @ThreadLeakScope(ThreadLeakScope.Scope.NONE) public abstract class SQLIntegTestCase extends RestIntegTestCase { @@ -51,8 +52,8 @@ protected void init() throws Exception { return; } - TestConfig config = new TestConfig(getCmdLineArgs()); - runner = new ComparisonTest(getThisDBConnection(config), + TestConfig config = new TestConfig(emptyMap()); + runner = new ComparisonTest(getESConnection(), getOtherDBConnections(config)); runner.connect(); @@ -67,7 +68,7 @@ protected void init() throws Exception { @AfterClass public static void cleanUp() { try { - TestConfig config = new TestConfig(getCmdLineArgs()); + TestConfig config = new TestConfig(emptyMap()); for (TestDataSet dataSet : config.getTestDataSets()) { runner.cleanUp(dataSet); } @@ -90,25 +91,6 @@ protected void verify(String... queries) { 0, summary.getFailure()); } - /** - * Execute a single given query and compare result with other database. - */ - protected void verify(String query) { - verify(new String[]{ query }); - } - - private static Map getCmdLineArgs() { - return Maps.fromProperties(System.getProperties()); - } - - private DBConnection getThisDBConnection(TestConfig config) { - String dbUrl = config.getDbConnectionUrl(); - if (dbUrl.isEmpty()) { - return getESConnection(); - } - return new JDBCConnection("DB Tested", dbUrl); - } - /** * Use Elasticsearch cluster initialized by ES Gradle task. */ From a887e89e61926c1adbf71bcc1fe33f5ebc846c18 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 26 Jun 2020 16:43:23 -0700 Subject: [PATCH 11/12] Prepare PR --- .../correctness/report/FailedTestCase.java | 2 +- .../runner/connection/ESConnection.java | 2 +- .../runner/resultset/DBResult.java | 38 ++++++++----------- .../sql/correctness/tests/DBResultTest.java | 2 +- 4 files changed, 19 insertions(+), 25 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java index c381ec6220..d6a209bde7 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/report/FailedTestCase.java @@ -52,7 +52,7 @@ public FailedTestCase(int id, String sql, List resultSets) { this.explain = resultSets.subList(1, resultSets.size()) .stream() .map(result -> resultSets.get(0).diff(result)) - .collect(Collectors.joining(",")); + .collect(Collectors.joining(", ")); } } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java index 8091d9e7e0..4ce4a7bde2 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/connection/ESConnection.java @@ -80,7 +80,7 @@ public DBResult select(String query) { @Override public void close() { // Only close database connection and leave ES REST connection alone - // because it's initialized and manged by outside. + // because it's initialized and manged by ES test base class. connection.close(); } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java index ccf9a24187..c8036e1a75 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java @@ -118,37 +118,31 @@ public String diff(DBResult other) { } private String diffSchema(DBResult other) { - if (schema.size() != other.schema.size()) { - return StringUtils.format( - "Schema size is different: this=[%d], other=[%d]", schema.size(), other.schema.size()); - } - - List thisTypes = new ArrayList<>(schema); - List otherTypes = new ArrayList<>(other.schema); - int diff = findFirstDifference(thisTypes, otherTypes); - if (diff >= 0) { - return StringUtils.format( - "Schema type at [%d] is different: thisType=[%s], otherType=[%s]", - diff, thisTypes.get(diff), otherTypes.get(diff)); - } - return ""; + List thisSchema = new ArrayList<>(schema); + List otherSchema = new ArrayList<>(other.schema); + return diff("Schema type", thisSchema, otherSchema); } private String diffDataRows(DBResult other) { List thisRows = sort(dataRows); List otherRows = sort(other.dataRows); + return diff("Data row", thisRows, otherRows); + } - if (thisRows.size() != otherRows.size()) { - return StringUtils.format( - "Data rows size is different: this=[%d], other=[%d]", - thisRows.size(), otherRows.size()); + /** + * Check if two lists are same otherwise explain if size or any element + * is different at some position. + */ + private String diff(String name, List thisList, List otherList) { + if (thisList.size() != otherList.size()) { + return StringUtils.format("%s size is different: this=[%d], other=[%d]", + name, thisList.size(), otherList.size()); } - int diff = findFirstDifference(thisRows, otherRows); + int diff = findFirstDifference(thisList, otherList); if (diff >= 0) { - return StringUtils.format( - "Data row at [%d] is different: this=[%s], other=[%s]", - diff, thisRows.get(diff), otherRows.get(diff)); + return StringUtils.format("%s at [%d] is different: this=[%s], other=[%s]", + name, diff, thisList.get(diff), otherList.get(diff)); } return ""; } diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java index 76f2d13178..ce52bcbf27 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java @@ -65,7 +65,7 @@ public void shouldExplainColumnTypeDifference() { assertEquals( "Schema type at [1] is different: " - + "thisType=[Type(name=age, type=FLOAT)], otherType=[Type(name=age, type=INT)]", + + "this=[Type(name=age, type=FLOAT)], other=[Type(name=age, type=INT)]", result1.diff(result2) ); } From 15f972d332b13ccdbd23823495158579f25fd08a Mon Sep 17 00:00:00 2001 From: Dai Date: Mon, 29 Jun 2020 08:14:43 -0700 Subject: [PATCH 12/12] Remove unused query files --- integ-test/src/test/resources/correctness/queries/from.txt | 0 integ-test/src/test/resources/correctness/queries/select.txt | 3 ++- integ-test/src/test/resources/correctness/queries/where.txt | 1 - 3 files changed, 2 insertions(+), 2 deletions(-) delete mode 100644 integ-test/src/test/resources/correctness/queries/from.txt delete mode 100644 integ-test/src/test/resources/correctness/queries/where.txt diff --git a/integ-test/src/test/resources/correctness/queries/from.txt b/integ-test/src/test/resources/correctness/queries/from.txt deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/integ-test/src/test/resources/correctness/queries/select.txt b/integ-test/src/test/resources/correctness/queries/select.txt index 3c7bf576fc..18d34e7ee9 100644 --- a/integ-test/src/test/resources/correctness/queries/select.txt +++ b/integ-test/src/test/resources/correctness/queries/select.txt @@ -1 +1,2 @@ -SELECT DistanceMiles FROM kibana_sample_data_flights \ No newline at end of file +SELECT DistanceMiles FROM kibana_sample_data_flights +SELECT AvgTicketPrice, Carrier FROM kibana_sample_data_flights WHERE AvgTicketPrice <= 500 diff --git a/integ-test/src/test/resources/correctness/queries/where.txt b/integ-test/src/test/resources/correctness/queries/where.txt deleted file mode 100644 index 9fce272bfa..0000000000 --- a/integ-test/src/test/resources/correctness/queries/where.txt +++ /dev/null @@ -1 +0,0 @@ -SELECT AvgTicketPrice, Carrier FROM kibana_sample_data_flights WHERE AvgTicketPrice <= 500 \ No newline at end of file