Enforce comparison test in new SQL module

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'

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,21 @@ public class FailedTestCase extends TestCaseReport {
    */
   private final List<DBResult> resultSets;
 
+  /**
+   * Explain where the difference is caused the test failure.
+   */
+  private final String explain;
+
   public FailedTestCase(int id, String sql, List<DBResult> 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(", "));
   }
 
 }

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<String[]> batch) {
-    Request request = new Request("POST", "/" + tableName + "/_bulk");
+    Request request = new Request("POST", "/" + tableName + "/_bulk?refresh=true");
     request.setJsonEntity(buildBulkBody(columnNames, batch));
     performRequest(request);
   }
@@ -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 ES test base class.
     connection.close();
-    try {
-      client.close();
-    } catch (IOException e) {
-      // Ignore
-    }
   }
 
   private void performRequest(Request request) {

integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/runner/resultset/DBResult.java

@@ -19,7 +19,9 @@
 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.List;
 import java.util.Set;
 import java.util.stream.Collectors;
 import lombok.EqualsAndHashCode;
@@ -101,4 +103,70 @@ public Collection<Collection<Object>> 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) {
+    List<Type> thisSchema = new ArrayList<>(schema);
+    List<Type> otherSchema = new ArrayList<>(other.schema);
+    return diff("Schema type", thisSchema, otherSchema);
+  }
+
+  private String diffDataRows(DBResult other) {
+    List<Row> thisRows = sort(dataRows);
+    List<Row> otherRows = sort(other.dataRows);
+    return diff("Data row", thisRows, otherRows);
+  }
+
+  /**
+   * 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(thisList, otherList);
+    if (diff >= 0) {
+      return StringUtils.format("%s at [%d] is different: this=[%s], other=[%s]",
+          name, diff, thisList.get(diff), otherList.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 <T extends Comparable<T>> List<T> sort(Collection<T> collection) {
+    ArrayList<T> list = new ArrayList<>(collection);
+    Collections.sort(list);
+    return list;
+  }
+
 }

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<Row> {
 
   private final Collection<Object> values;
 
@@ -56,4 +57,32 @@ private Object roundFloatNum(Object value) {
     return value;
   }
 
+  @SuppressWarnings("unchecked")
+  @Override
+  public int compareTo(Row other) {
+    List<Object> thisObjects = new ArrayList<>(values);
+    List<Object> 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;
+  }
+
 }

integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/correctness/tests/DBResultTest.java

@@ -20,7 +20,10 @@
 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 org.junit.Test;
 
@@ -53,4 +56,37 @@ public void dbResultWithDifferentColumnTypeShouldNotEqual() {
     assertNotEquals(result1, result2);
   }
 
+  @Test
+  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",
+        Arrays.asList(new Type("name", "VARCHAR"), new Type("age", "INT")), emptyList());
+
+    assertEquals(
+        "Schema type at [1] is different: "
+            + "this=[Type(name=age, type=FLOAT)], other=[Type(name=age, type=INT)]",
+        result1.diff(result2)
+    );
+  }
+
+  @Test
+  public void shouldExplainDataRowsDifference() {
+    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)
+    );
+  }
+
 }

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"

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));
+  }
+
 }

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\"," +

integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/SQLCorrectnessIT.java

@@ -0,0 +1,67 @@
+/*
+ *    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.Function;
+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"*/ }; //TODO: skip bugfixes folder for now since it fails
+
+  @Test
+  public void runAllTests() throws Exception {
+    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 verifyQueries(String[] dirs, Function<String, String> 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 -> verifyQueries(file, converter));
+    }
+  }
+
+  private void verifyQueries(Path file, Function<String, String> 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);
+    }
+  }
+
+}