Skip to content

Minor JDBC driver improvements #6979

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
electrum merged 5 commits into from
Feb 28, 2017
Merged

Minor JDBC driver improvements #6979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
afa661d
Update driver JDBC version number to 4.2
electrum Dec 28, 2016
1172956
Update JDBC feature flags for recently added features
electrum Dec 29, 2016
f43ca0d
Return server major/minor version in JDBC driver
electrum Dec 29, 2016
b420d67
Return correct JDBC driver version
electrum Dec 28, 2016
c57e7a3
Rename JDBC TestDriver to TestPrestoDriver
electrum Dec 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public class PrestoConnection
private final AtomicReference<String> schema = new AtomicReference<>();
private final AtomicReference<String> timeZoneId = new AtomicReference<>();
private final AtomicReference<Locale> locale = new AtomicReference<>();
private final AtomicReference<ServerInfo> serverInfo = new AtomicReference<>();

private final URI jdbcUri;
private final URI httpUri;
Expand Down Expand Up @@ -566,8 +567,17 @@ String getUser()
}

ServerInfo getServerInfo()
throws SQLException
{
return queryExecutor.getServerInfo(httpUri);
if (serverInfo.get() == null) {
try {
serverInfo.set(queryExecutor.getServerInfo(httpUri));
}
catch (RuntimeException e) {
throw new SQLException("Error fetching version from server", e);
}
}
return serverInfo.get();
}

StatementClient startQuery(String sql)
Expand Down
57 changes: 30 additions & 27 deletions presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoDatabaseMetaData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.facebook.presto.jdbc;

import com.google.common.base.Joiner;
import com.google.common.base.Splitter;

import java.sql.Connection;
import java.sql.DatabaseMetaData;
Expand All @@ -25,6 +26,7 @@
import java.util.ArrayList;
import java.util.List;

import static java.lang.Integer.parseInt;
import static java.util.Objects.requireNonNull;

public class PrestoDatabaseMetaData
Expand Down Expand Up @@ -113,12 +115,7 @@ public String getDatabaseProductName()
public String getDatabaseProductVersion()
throws SQLException
{
try {
return connection.getServerInfo().getNodeVersion().getVersion();
}
catch (RuntimeException e) {
throw new SQLException("Error fetching version from server", e);
}
return connection.getServerInfo().getNodeVersion().getVersion();
}

@Override
Expand All @@ -138,13 +135,13 @@ public String getDriverVersion()
@Override
public int getDriverMajorVersion()
{
return PrestoDriver.VERSION_MAJOR;
return PrestoDriver.DRIVER_VERSION_MAJOR;
}

@Override
public int getDriverMinorVersion()
{
return PrestoDriver.VERSION_MINOR;
return PrestoDriver.DRIVER_VERSION_MINOR;
}

@Override
Expand Down Expand Up @@ -602,24 +599,22 @@ public boolean supportsSelectForUpdate()
public boolean supportsStoredProcedures()
throws SQLException
{
// TODO: support stored procedures
// TODO: support stored procedure escape syntax
return false;
}

@Override
public boolean supportsSubqueriesInComparisons()
throws SQLException
{
// TODO: support subqueries in comparisons
return false;
return true;
}

@Override
public boolean supportsSubqueriesInExists()
throws SQLException
{
// TODO: support EXISTS
return false;
return true;
}

@Override
Expand All @@ -633,16 +628,14 @@ public boolean supportsSubqueriesInIns()
public boolean supportsSubqueriesInQuantifieds()
throws SQLException
{
// TODO: support subqueries in ANY/SOME/ALL predicates
return false;
return true;
}

@Override
public boolean supportsCorrelatedSubqueries()
throws SQLException
{
// TODO: support correlated subqueries
return false;
return true;
}

@Override
Expand Down Expand Up @@ -844,23 +837,21 @@ public int getMaxUserNameLength()
public int getDefaultTransactionIsolation()
throws SQLException
{
// TODO: support transactions
return Connection.TRANSACTION_NONE;
return Connection.TRANSACTION_READ_UNCOMMITTED;
}

@Override
public boolean supportsTransactions()
throws SQLException
{
// TODO: support transactions
return false;
return true;
}

@Override
public boolean supportsTransactionIsolationLevel(int level)
throws SQLException
{
return level == Connection.TRANSACTION_NONE;
return true;
}

@Override
Expand Down Expand Up @@ -1249,29 +1240,41 @@ public int getResultSetHoldability()
public int getDatabaseMajorVersion()
throws SQLException
{
// TODO: get version from server
return PrestoDriver.VERSION_MAJOR;
return getDatabaseVersionPart(0);
}

@Override
public int getDatabaseMinorVersion()
throws SQLException
{
return PrestoDriver.VERSION_MINOR;
return getDatabaseVersionPart(1);
}

private int getDatabaseVersionPart(int part)
throws SQLException
{
String version = getDatabaseProductVersion();
List<String> parts = Splitter.on('.').limit(3).splitToList(version);
try {
return parseInt(parts.get(part));
}
catch (IndexOutOfBoundsException | NumberFormatException e) {
return 0;
Copy link
Copy Markdown
Contributor

@ebd2 ebd2 Jan 3, 2017

Choose a reason for hiding this comment

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

2 minor comments/questions:

  1. Will this return 0 for every snapshot version when getDatabaseMinorVersion() is called? If so, that seems like a pain for us or anybody doing testing.
  2. For end users, it seems likely that the only reason you'd hit this is because the presto versioning scheme has radically changed and the driver version parsing code is out of date. It seems like a good bet that the user needs to update the driver. How about throwing an SQLException containing the full version string like "Driver version x.y could not parse server version string '%s'. Try updating the driver"

Please feel free to ignore if this is spec-compliant behavior as-is.

Copy link
Copy Markdown
Contributor Author

@electrum electrum Jan 4, 2017

Choose a reason for hiding this comment

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

  1. If the driver JAR is built from Maven, it will work correctly, since the snapshot JAR will have the correct manifest. I just tested this out (and fixed a bug). The version for snapshots is a bit weird -- it's the output of git describe HEAD. So, for this branch on 0.162-SNAPSHOT, the version is 0.161-115-gea22394b5 (and that depends on having fetched the tags to the local Git repo). Thus, you get 0/161 for major/minor, and the full string for getDriverVersion().
  2. I considered that, but it will break for stuff like presto.version=testversion that we have in the development config.properties file. I'd rather be conservative for now.

Copy link
Copy Markdown
Contributor Author

@electrum electrum Jan 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I just realized my comment for #1 is actually about the JDBC driver version, not the server version. However, the answer is nearly the same. The server version is determined the same way, from the package manifest, unless explicitly overridden with presto.version.

}
}

@Override
public int getJDBCMajorVersion()
throws SQLException
{
return PrestoDriver.JDBC_VERSION_MAJOR;
return 4;
}

@Override
public int getJDBCMinorVersion()
throws SQLException
{
return PrestoDriver.JDBC_VERSION_MINOR;
return 2;
}

@Override
Expand Down
31 changes: 22 additions & 9 deletions presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
*/
package com.facebook.presto.jdbc;

import com.google.common.base.CharMatcher;
import com.google.common.base.Splitter;
import com.google.common.base.Throwables;

import java.io.Closeable;
Expand All @@ -22,23 +24,21 @@
import java.sql.DriverPropertyInfo;
import java.sql.SQLException;
import java.sql.SQLFeatureNotSupportedException;
import java.util.List;
import java.util.Properties;
import java.util.logging.Logger;

import static com.google.common.base.Strings.isNullOrEmpty;
import static java.lang.Integer.parseInt;
import static java.lang.String.format;

public class PrestoDriver
implements Driver, Closeable
{
static final int VERSION_MAJOR = 1;
static final int VERSION_MINOR = 0;

static final int JDBC_VERSION_MAJOR = 4;
static final int JDBC_VERSION_MINOR = 1;

static final String DRIVER_NAME = "Presto JDBC Driver";
static final String DRIVER_VERSION = VERSION_MAJOR + "." + VERSION_MINOR;
static final String DRIVER_VERSION;
static final int DRIVER_VERSION_MAJOR;
static final int DRIVER_VERSION_MINOR;

private static final DriverPropertyInfo[] DRIVER_PROPERTY_INFOS = {};

Expand All @@ -49,6 +49,19 @@ public class PrestoDriver
private final QueryExecutor queryExecutor;

static {
String version = PrestoDriver.class.getPackage().getImplementationVersion();
if (version == null) {
DRIVER_VERSION = "unknown";
DRIVER_VERSION_MAJOR = 0;
DRIVER_VERSION_MINOR = 0;
}
else {
DRIVER_VERSION = version;
List<String> parts = Splitter.on(CharMatcher.anyOf(".-")).limit(3).splitToList(version);
DRIVER_VERSION_MAJOR = parseInt(parts.get(0));
DRIVER_VERSION_MINOR = parseInt(parts.get(1));
}

try {
DriverManager.registerDriver(new PrestoDriver());
}
Expand Down Expand Up @@ -101,13 +114,13 @@ public DriverPropertyInfo[] getPropertyInfo(String url, Properties info)
@Override
public int getMajorVersion()
{
return VERSION_MAJOR;
return DRIVER_VERSION_MAJOR;
}

@Override
public int getMinorVersion()
{
return VERSION_MINOR;
return DRIVER_VERSION_MINOR;
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion presto-jdbc/src/test/java/com/facebook/presto/jdbc/TestJdbcResultSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import java.sql.Statement;
import java.sql.Types;

import static com.facebook.presto.jdbc.TestDriver.closeQuietly;
import static com.facebook.presto.jdbc.TestPrestoDriver.closeQuietly;
import static java.lang.String.format;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
Expand Down
27 changes: 25 additions & 2 deletions .../com/facebook/presto/jdbc/TestDriver.java → ...acebook/presto/jdbc/TestPrestoDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@

import java.math.BigDecimal;
import java.sql.Connection;
import java.sql.DatabaseMetaData;
import java.sql.Date;
import java.sql.Driver;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
Expand Down Expand Up @@ -92,7 +94,7 @@
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;

public class TestDriver
public class TestPrestoDriver
{
private static final DateTimeZone ASIA_ORAL_ZONE = DateTimeZone.forID("Asia/Oral");
private static final GregorianCalendar ASIA_ORAL_CALENDAR = new GregorianCalendar(ASIA_ORAL_ZONE.toTimeZone());
Expand Down Expand Up @@ -371,12 +373,33 @@ public void testGetCatalogs()
}
}

@Test
public void testGetDriverVersion()
throws Exception
{
Driver driver = DriverManager.getDriver("jdbc:presto:");
assertEquals(driver.getMajorVersion(), 0);
assertEquals(driver.getMajorVersion(), 0);

try (Connection connection = createConnection()) {
DatabaseMetaData metaData = connection.getMetaData();
assertEquals(metaData.getDriverName(), PrestoDriver.DRIVER_NAME);
assertEquals(metaData.getDriverVersion(), "unknown");
assertEquals(metaData.getDriverMajorVersion(), 0);
assertEquals(metaData.getDriverMinorVersion(), 0);
}
}

@Test
public void testGetDatabaseProductVersion()
throws Exception
{
try (Connection connection = createConnection()) {
assertNotNull(connection.getMetaData().getDatabaseProductVersion());
DatabaseMetaData metaData = connection.getMetaData();
assertEquals(metaData.getDatabaseProductName(), "Presto");
assertEquals(metaData.getDatabaseProductVersion(), "testversion");
assertEquals(metaData.getDatabaseMajorVersion(), 0);
assertEquals(metaData.getDatabaseMinorVersion(), 0);
}
}

Expand Down