From e2371f5812d3318845bdd063f3f1208cf92f7781 Mon Sep 17 00:00:00 2001 From: Lyndon Bauto Date: Wed, 24 Nov 2021 15:08:03 -0800 Subject: [PATCH 1/5] [1] Fix for inconsistent LIMIT 1 bug --- build.gradle | 3 +- .../results/SqlGremlinQueryResult.java | 69 ++++++------------- .../results/pagination/Pagination.java | 2 +- .../gremlin/adapter/GremlinSqlBaseTest.java | 16 ++--- .../gremlin/sql/SqlGremlinResultSet.java | 16 ++--- .../jdbc/utilities/ConnectionProperties.java | 2 +- 6 files changed, 35 insertions(+), 73 deletions(-) diff --git a/build.gradle b/build.gradle index 57a80bab..626f8899 100644 --- a/build.gradle +++ b/build.gradle @@ -147,6 +147,7 @@ task writeProperties(type: WriteProperties) { jacoco { toolVersion = "0.8.5" } + jacocoTestReport { reports { html.enabled true @@ -259,7 +260,7 @@ dependencies { // Testing testImplementation group: 'com.googlecode.json-simple', name: 'json-simple', version: '1.1.1' - testImplementation group: 'junit', name: 'junit', version: '4.13.2' + testImplementation group: 'junit', name: 'junit', version: '4.13.2' testImplementation group: 'org.junit.jupiter', name: 'junit-jupiter-api', version: jupiterVersion testImplementation group: 'org.junit.jupiter', name: 'junit-jupiter-engine', version: jupiterVersion testImplementation group: 'org.mockito', name: 'mockito-core', version: '4.0.0' diff --git a/sql-gremlin/src/main/java/org/twilmes/sql/gremlin/adapter/results/SqlGremlinQueryResult.java b/sql-gremlin/src/main/java/org/twilmes/sql/gremlin/adapter/results/SqlGremlinQueryResult.java index 72192ee7..b47a8af6 100644 --- a/sql-gremlin/src/main/java/org/twilmes/sql/gremlin/adapter/results/SqlGremlinQueryResult.java +++ b/sql-gremlin/src/main/java/org/twilmes/sql/gremlin/adapter/results/SqlGremlinQueryResult.java @@ -21,7 +21,6 @@ import lombok.Getter; import org.twilmes.sql.gremlin.adapter.converter.SqlMetadata; -import org.twilmes.sql.gremlin.adapter.converter.schema.gremlin.GremlinProperty; import org.twilmes.sql.gremlin.adapter.converter.schema.gremlin.GremlinTableBase; import java.sql.SQLException; import java.util.ArrayList; @@ -30,16 +29,13 @@ import java.util.concurrent.LinkedBlockingQueue; @Getter -public class SqlGremlinQueryResult { +public class SqlGremlinQueryResult implements AutoCloseable { public static final String EMPTY_MESSAGE = "No more results."; public static final String NULL_VALUE = "$%#NULL#%$"; private final List columns; private final List columnTypes = new ArrayList<>(); - private final Object assertEmptyLock = new Object(); private final BlockingQueue> blockingQueueRows = new LinkedBlockingQueue<>(); - private boolean isEmpty = false; private SQLException paginationException = null; - private Thread currThread = null; public SqlGremlinQueryResult(final List columns, final List gremlinTableBases, final SqlMetadata sqlMetadata) throws SQLException { @@ -49,7 +45,8 @@ public SqlGremlinQueryResult(final List columns, final List gremlinTableBases) throws SQLException { + private String getType(final String column, final SqlMetadata sqlMetadata, + final List gremlinTableBases) throws SQLException { if (sqlMetadata.aggregateTypeExists(column)) { return sqlMetadata.getOutputType(column, "string"); } @@ -67,60 +64,38 @@ private String getType(final String column, final SqlMetadata sqlMetadata, final } public void setPaginationException(final SQLException e) { - synchronized (assertEmptyLock) { - paginationException = e; - if (currThread != null && blockingQueueRows.size() == 0) { - currThread.interrupt(); - } - } - } - - public boolean getIsEmpty() throws SQLException { - if (paginationException == null) { - return isEmpty; - } - throw paginationException; + paginationException = e; + close(); } - public void assertIsEmpty() { - synchronized (assertEmptyLock) { - if (currThread != null && blockingQueueRows.size() == 0) { - currThread.interrupt(); - } - isEmpty = true; - } + @Override + public void close() { + blockingQueueRows.add(new EmptyResult()); } public void addResults(final List> rows) { - for (final List row : rows) { - for (int i = 0; i < row.size(); i++) { - if (row.get(i) instanceof String && row.get(i).toString().equals(NULL_VALUE)) { - row.set(i, null); - } - } - } + // This is a workaround for Gremlin null support not being in any version of Gremlin that is + // widely supported by database vendors. + rows.forEach(row -> row.replaceAll(col -> (col instanceof String && col.equals(NULL_VALUE) ? null : col))); blockingQueueRows.addAll(rows); } - private boolean getShouldExit() throws SQLException { - synchronized (assertEmptyLock) { - return (getIsEmpty() && blockingQueueRows.size() == 0); - } - } - - public Object getResult() throws SQLException { - synchronized (assertEmptyLock) { - this.currThread = Thread.currentThread(); - } - while (!getShouldExit()) { + public List getResult() throws SQLException { + while (true) { try { - return this.blockingQueueRows.take(); - } catch (final InterruptedException ignored) { + final List result = blockingQueueRows.take(); + + // If a pagination exception occurs, an EmptyResult Object will be inserted into the BlockingQueue. + // The pagination exception needs to be checked before returning. if (paginationException != null) { throw paginationException; } + return result; + } catch (final InterruptedException ignored) { } } - throw new SQLException(EMPTY_MESSAGE); + } + + public static class EmptyResult extends ArrayList { } } diff --git a/sql-gremlin/src/main/java/org/twilmes/sql/gremlin/adapter/results/pagination/Pagination.java b/sql-gremlin/src/main/java/org/twilmes/sql/gremlin/adapter/results/pagination/Pagination.java index d77c0dd9..f01af629 100644 --- a/sql-gremlin/src/main/java/org/twilmes/sql/gremlin/adapter/results/pagination/Pagination.java +++ b/sql-gremlin/src/main/java/org/twilmes/sql/gremlin/adapter/results/pagination/Pagination.java @@ -59,7 +59,7 @@ public void run() { convertAndInsertResult(sqlGremlinQueryResult, rows); } // If we run out of traversal data (or hit our limit), stop and signal to the result that it is done. - sqlGremlinQueryResult.assertIsEmpty(); + sqlGremlinQueryResult.close(); } catch (final Exception e) { final StringWriter sw = new StringWriter(); final PrintWriter pw = new PrintWriter(sw); diff --git a/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlBaseTest.java b/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlBaseTest.java index 8936bbb8..8c2a8377 100644 --- a/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlBaseTest.java +++ b/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlBaseTest.java @@ -151,20 +151,12 @@ static class SqlGremlinTestResult { SqlGremlinTestResult(final SqlGremlinQueryResult sqlGremlinQueryResult) throws SQLException { columns = sqlGremlinQueryResult.getColumns(); - Object res; do { - try { - res = sqlGremlinQueryResult.getResult(); - } catch (final SQLException e) { - if (e.getMessage().equals(SqlGremlinQueryResult.EMPTY_MESSAGE)) { - break; - } else { - throw e; - } - } - if (res instanceof List) { - this.rows.add((List) res); + final List res = sqlGremlinQueryResult.getResult(); + if (res instanceof SqlGremlinQueryResult.EmptyResult) { + break; } + this.rows.add(res); } while (true); } } diff --git a/src/main/java/software/aws/neptune/gremlin/sql/SqlGremlinResultSet.java b/src/main/java/software/aws/neptune/gremlin/sql/SqlGremlinResultSet.java index 76f48df7..43c72e32 100644 --- a/src/main/java/software/aws/neptune/gremlin/sql/SqlGremlinResultSet.java +++ b/src/main/java/software/aws/neptune/gremlin/sql/SqlGremlinResultSet.java @@ -19,9 +19,9 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.twilmes.sql.gremlin.adapter.results.SqlGremlinQueryResult; +import software.aws.neptune.common.EmptyResultSet; import software.aws.neptune.common.gremlindatamodel.resultset.ResultSetGetColumns; import software.aws.neptune.gremlin.GremlinTypeMapping; -import software.aws.neptune.gremlin.resultset.GremlinResultSet; import software.aws.neptune.gremlin.resultset.GremlinResultSetMetadata; import software.aws.neptune.jdbc.ResultSet; import software.aws.neptune.jdbc.utilities.SqlError; @@ -33,7 +33,7 @@ import java.util.Optional; public class SqlGremlinResultSet extends ResultSet implements java.sql.ResultSet { - private static final Logger LOGGER = LoggerFactory.getLogger(GremlinResultSet.class); + private static final Logger LOGGER = LoggerFactory.getLogger(SqlGremlinResultSet.class); private final List columns; private final List columnTypes; @@ -79,15 +79,9 @@ protected void doClose() throws SQLException { @Override public boolean next() throws SQLException { final Object res; - try { - res = sqlQueryResult.getResult(); - } catch (final SQLException e) { - if (e.getMessage().equals(SqlGremlinQueryResult.EMPTY_MESSAGE)) { - LOGGER.trace(SqlGremlinQueryResult.EMPTY_MESSAGE); - return false; - } else { - throw e; - } + res = sqlQueryResult.getResult(); + if (res instanceof SqlGremlinQueryResult.EmptyResult) { + return false; } this.row = (List) res; return true; diff --git a/src/main/java/software/aws/neptune/jdbc/utilities/ConnectionProperties.java b/src/main/java/software/aws/neptune/jdbc/utilities/ConnectionProperties.java index c1937543..cd1e0a5c 100644 --- a/src/main/java/software/aws/neptune/jdbc/utilities/ConnectionProperties.java +++ b/src/main/java/software/aws/neptune/jdbc/utilities/ConnectionProperties.java @@ -59,7 +59,7 @@ public abstract class ConnectionProperties extends Properties { public static final int DEFAULT_CONNECTION_TIMEOUT_MILLIS = 5000; public static final int DEFAULT_CONNECTION_RETRY_COUNT = 3; public static final String DEFAULT_SSH_STRICT_CHECKING = "true"; - public static final Level DEFAULT_LOG_LEVEL = Level.OFF; + public static final Level DEFAULT_LOG_LEVEL = Level.ALL; public static final String DEFAULT_SERVICE_REGION = ""; public static final Map DEFAULT_PROPERTIES_MAP = new HashMap<>(); From 97b49752bb934ee7a323d4f64dba118efa90a621 Mon Sep 17 00:00:00 2001 From: Lyndon Bauto Date: Wed, 24 Nov 2021 15:08:38 -0800 Subject: [PATCH 2/5] [1] Adding tests --- .../adapter/GremlinSqlAdvancedSelectTest.java | 64 ++++++++++++++++--- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlAdvancedSelectTest.java b/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlAdvancedSelectTest.java index df78a08a..64dd437f 100644 --- a/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlAdvancedSelectTest.java +++ b/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlAdvancedSelectTest.java @@ -241,36 +241,82 @@ void testMisc() throws SQLException { rows(r("Tom"), r("Susan"), r("Phil"), r("Pavel"), r("Patty"), r("Juanita"))); // NULLS FIRST predicate is not currently supported. - runQueryTestThrows("SELECT name FROM \"gremlin\".\"person\" ORDER BY name NULLS FIRST", "Error, no appropriate order for GremlinSqlPostFixOperator of NULLS_FIRST."); + runQueryTestThrows("SELECT name FROM \"gremlin\".\"person\" ORDER BY name NULLS FIRST", + "Error, no appropriate order for GremlinSqlPostFixOperator of NULLS_FIRST."); } @Test void testAggregateLiteralHavingNoGroupBy() throws SQLException { // Tableau was sending queries like this for the preview in 2021.3 - runQueryTestResults("SELECT SUM(1) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", + runQueryTestResults( + "SELECT SUM(1) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", columns("cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok"), rows(r(new BigDecimal(6)))); - runQueryTestResults("SELECT MIN(1) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", + runQueryTestResults( + "SELECT MIN(1) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", columns("cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok"), rows(r(new BigDecimal(1)))); - runQueryTestResults("SELECT MAX(1) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", + runQueryTestResults( + "SELECT MAX(1) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", columns("cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok"), rows(r(new BigDecimal(1)))); - runQueryTestResults("SELECT AVG(1) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", + runQueryTestResults( + "SELECT AVG(1) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", columns("cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok"), rows(r(new BigDecimal(1)))); - runQueryTestResults("SELECT SUM(2) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", + runQueryTestResults( + "SELECT SUM(2) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", columns("cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok"), rows(r(new BigDecimal(12)))); - runQueryTestResults("SELECT MIN(2) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", + runQueryTestResults( + "SELECT MIN(2) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", columns("cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok"), rows(r(new BigDecimal(2)))); - runQueryTestResults("SELECT MAX(2) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", + runQueryTestResults( + "SELECT MAX(2) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", columns("cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok"), rows(r(new BigDecimal(2)))); - runQueryTestResults("SELECT AVG(2) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", + runQueryTestResults( + "SELECT AVG(2) AS \"cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok\" FROM gremlin.person AS person HAVING COUNT(1) > 0", columns("cnt:airport_03C2E834E28942D3AA2423AC01F4B33D:ok"), rows(r(new BigDecimal(2)))); } + + @Test + void testLimit() throws SQLException { + // LIMIT 1 tests. + // Single result query. + runQueryTestResults("SELECT name, age FROM person WHERE name = 'Tom' ORDER BY age LIMIT 1", + columns("name", "age"), + rows(r("Tom", 35))); + // Multi result query. + runQueryTestResults("SELECT name, age FROM person ORDER BY age LIMIT 1", + columns("name", "age"), + rows(r("Patty", 29))); + + // LIMIT > 1 tests. + runQueryTestResults("SELECT name, age FROM person WHERE name <> 'Tom' ORDER BY age LIMIT 2", + columns("name", "age"), + rows(r("Patty", 29), r("Pavel", 30))); + runQueryTestResults("SELECT name, age FROM person WHERE name <> 'Tom' ORDER BY age LIMIT 3", + columns("name", "age"), + rows(r("Patty", 29), r("Pavel", 30), r("Phil", 31))); + runQueryTestResults("SELECT name, age FROM person WHERE name <> 'Tom' ORDER BY age LIMIT 4", + columns("name", "age"), + rows(r("Patty", 29), r("Pavel", 30), r("Phil", 31), r("Susan", 45))); + runQueryTestResults("SELECT name, age FROM person WHERE name <> 'Tom' ORDER BY age LIMIT 5", + columns("name", "age"), + rows(r("Patty", 29), r("Pavel", 30), r("Phil", 31), r("Susan", 45), r("Juanita", 50))); + runQueryTestResults("SELECT name, age FROM person WHERE name <> 'Tom' ORDER BY age LIMIT 6", + columns("name", "age"), + rows(r("Patty", 29), r("Pavel", 30), r("Phil", 31), r("Susan", 45), r("Juanita", 50))); + runQueryTestResults("SELECT name, age FROM person WHERE name <> 'Tom' ORDER BY age LIMIT 1000", + columns("name", "age"), + rows(r("Patty", 29), r("Pavel", 30), r("Phil", 31), r("Susan", 45), r("Juanita", 50))); + runQueryTestResults( + String.format("SELECT name, age FROM person WHERE name <> 'Tom' ORDER BY age LIMIT %d", Long.MAX_VALUE), + columns("name", "age"), + rows(r("Patty", 29), r("Pavel", 30), r("Phil", 31), r("Susan", 45), r("Juanita", 50))); + } } From 74a7199e995a9462993e9ffd13a538a60f10d80b Mon Sep 17 00:00:00 2001 From: Lyndon Bauto Date: Wed, 24 Nov 2021 15:47:08 -0800 Subject: [PATCH 3/5] [1] Fixing unused import in CheckStyle. --- .../software/aws/neptune/gremlin/sql/SqlGremlinResultSet.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/software/aws/neptune/gremlin/sql/SqlGremlinResultSet.java b/src/main/java/software/aws/neptune/gremlin/sql/SqlGremlinResultSet.java index 43c72e32..cf5930a1 100644 --- a/src/main/java/software/aws/neptune/gremlin/sql/SqlGremlinResultSet.java +++ b/src/main/java/software/aws/neptune/gremlin/sql/SqlGremlinResultSet.java @@ -19,7 +19,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.twilmes.sql.gremlin.adapter.results.SqlGremlinQueryResult; -import software.aws.neptune.common.EmptyResultSet; import software.aws.neptune.common.gremlindatamodel.resultset.ResultSetGetColumns; import software.aws.neptune.gremlin.GremlinTypeMapping; import software.aws.neptune.gremlin.resultset.GremlinResultSetMetadata; From ac7f881e953a109a01ec01c6457446a01051d77d Mon Sep 17 00:00:00 2001 From: Lyndon Bauto Date: Wed, 24 Nov 2021 15:54:32 -0800 Subject: [PATCH 4/5] [1] Reverting unintentional commit. --- .../aws/neptune/jdbc/utilities/ConnectionProperties.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/software/aws/neptune/jdbc/utilities/ConnectionProperties.java b/src/main/java/software/aws/neptune/jdbc/utilities/ConnectionProperties.java index cd1e0a5c..c1937543 100644 --- a/src/main/java/software/aws/neptune/jdbc/utilities/ConnectionProperties.java +++ b/src/main/java/software/aws/neptune/jdbc/utilities/ConnectionProperties.java @@ -59,7 +59,7 @@ public abstract class ConnectionProperties extends Properties { public static final int DEFAULT_CONNECTION_TIMEOUT_MILLIS = 5000; public static final int DEFAULT_CONNECTION_RETRY_COUNT = 3; public static final String DEFAULT_SSH_STRICT_CHECKING = "true"; - public static final Level DEFAULT_LOG_LEVEL = Level.ALL; + public static final Level DEFAULT_LOG_LEVEL = Level.OFF; public static final String DEFAULT_SERVICE_REGION = ""; public static final Map DEFAULT_PROPERTIES_MAP = new HashMap<>(); From fbe7396c576eb569cc67cc2f413aa5e2e4321ac5 Mon Sep 17 00:00:00 2001 From: Lyndon Bauto Date: Fri, 26 Nov 2021 14:41:27 -0800 Subject: [PATCH 5/5] [1] Fixing loop. --- .../sql/gremlin/adapter/GremlinSqlBaseTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlBaseTest.java b/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlBaseTest.java index 8c2a8377..acd4ec2b 100644 --- a/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlBaseTest.java +++ b/sql-gremlin/src/test/java/org/twilmes/sql/gremlin/adapter/GremlinSqlBaseTest.java @@ -151,13 +151,13 @@ static class SqlGremlinTestResult { SqlGremlinTestResult(final SqlGremlinQueryResult sqlGremlinQueryResult) throws SQLException { columns = sqlGremlinQueryResult.getColumns(); + List res; do { - final List res = sqlGremlinQueryResult.getResult(); - if (res instanceof SqlGremlinQueryResult.EmptyResult) { - break; + res = sqlGremlinQueryResult.getResult(); + if (!(res instanceof SqlGremlinQueryResult.EmptyResult)) { + this.rows.add(res); } - this.rows.add(res); - } while (true); + } while (!(res instanceof SqlGremlinQueryResult.EmptyResult)); } } }