Address all JDBC ratification comments with small fixes #1

java/flight/flight-jdbc-driver/jdbc-spotbugs-exclude.xml

@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<FindBugsFilter>
+    <!--  These elements are supposed to be mutable  -->
+    <Match>
+        <Package name="~org\.apache\.arrow\.driver\.jdbc\.accessor\.impl.*"/>
+        <Bug pattern="EI_EXPOSE_REP2"/>
+    </Match>
+    <Match>
+        <Class name="org.apache.arrow.driver.jdbc.client.ArrowFlightSqlClientHandler"/>
+        <Bug pattern="EI_EXPOSE_REP2"/>
+    </Match>
+    <Match>
+        <Class name="org.apache.arrow.driver.jdbc.utils.ConnectionWrapper"/>
+        <Bug pattern="EI_EXPOSE_REP2"/>
+    </Match>
+    <Match>
+        <Class name="org.apache.arrow.driver.jdbc.ArrowFlightJdbcDataSource"/>
+        <Bug pattern="EI_EXPOSE_REP2"/>
+    </Match>
+    <Match>
+        <Class name="org.apache.arrow.driver.jdbc.ArrowFlightJdbcCursor"/>
+        <Bug pattern="EI_EXPOSE_REP2"/>
+    </Match>
+
+    <Match>
+        <Class name="org.apache.arrow.driver.jdbc.ArrowFlightJdbcDataSource"/>
+        <Bug pattern="EI_EXPOSE_REP"/>
+    </Match>
+</FindBugsFilter>

java/flight/flight-jdbc-driver/pom.xml

@@ -79,16 +79,6 @@
             <classifier>${arrow.vector.classifier}</classifier>
             </dependency>
 
-        <dependency>
-            <groupId>org.apache.calcite.avatica</groupId>
-            <artifactId>avatica</artifactId>
-		</dependency>
-
-        <dependency>
-            <groupId>org.bouncycastle</groupId>
-            <artifactId>bcpkix-jdk15on</artifactId>
-        </dependency>
-
         <dependency>
             <groupId>com.google.guava</groupId>
             <artifactId>guava</artifactId>
@@ -142,6 +132,17 @@
             <artifactId>flight-sql</artifactId>
             <version>${project.version}</version>
         </dependency>
+
+        <dependency>
+            <groupId>org.apache.calcite.avatica</groupId>
+            <artifactId>avatica</artifactId>
+            <version>1.18.0</version>
+        </dependency>
+        <dependency>
+            <groupId>org.bouncycastle</groupId>
+            <artifactId>bcpkix-jdk15on</artifactId>
+            <version>1.61</version>
+        </dependency>
     </dependencies>
 
     <build>
@@ -171,6 +172,7 @@
                 <groupId>com.github.spotbugs</groupId>
                 <artifactId>spotbugs-maven-plugin</artifactId>
                 <configuration>
+                    <excludeFilterFile>jdbc-spotbugs-exclude.xml</excludeFilterFile>
                     <xmlOutput>true</xmlOutput>
                     <xmlOutputDirectory>target/spotbugs</xmlOutputDirectory>
                 </configuration>

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/ArrowDatabaseMetadata.java

@@ -729,8 +729,8 @@ private <T> T getSqlInfoAndCacheIfCacheIsEmpty(final SqlInfo sqlInfoCommand,
                                                  final Class<T> desiredType)
       throws SQLException {
     final ArrowFlightConnection connection = getConnection();
-    final FlightInfo sqlInfo = connection.getClientHandler().getSqlInfo();
     if (cachedSqlInfo.isEmpty()) {
+      final FlightInfo sqlInfo = connection.getClientHandler().getSqlInfo();
       synchronized (cachedSqlInfo) {
         if (cachedSqlInfo.isEmpty()) {
           try (final ResultSet resultSet =

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightConnection.java

@@ -104,7 +104,11 @@ private static ArrowFlightSqlClientHandler createNewClientHandler(
           .withCallOptions(config.toCallOption())
           .build();
     } catch (final SQLException e) {
-      allocator.close();
+      try {
+        allocator.close();
+      } catch (final Exception allocatorCloseEx) {
+        e.addSuppressed(allocatorCloseEx);
+      }
       throw e;
     }
   }
@@ -149,7 +153,9 @@ synchronized ExecutorService getExecutorService() {
 
   @Override
   public Properties getClientInfo() {
-    return info;
+    final Properties copy = new Properties();
+    copy.putAll(info);
+    return copy;
   }
 
   @Override

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcCursor.java

@@ -78,7 +78,7 @@ private Accessor createAccessor(FieldVector vector) {
    */
   @Override
   protected Getter createGetter(int column) {
-    throw new UnsupportedOperationException();
+    throw new UnsupportedOperationException("Not allowed.");
   }
 
   @Override

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcDataSource.java

@@ -103,17 +103,17 @@ public <T> T unwrap(Class<T> aClass) throws SQLException {
   }
 
   @Override
-  public boolean isWrapperFor(Class<?> aClass) throws SQLException {
+  public boolean isWrapperFor(Class<?> aClass) {
     return false;
   }
 
   @Override
-  public PrintWriter getLogWriter() throws SQLException {
+  public PrintWriter getLogWriter() {
     return this.logWriter;
   }
 
   @Override
-  public void setLogWriter(PrintWriter logWriter) throws SQLException {
+  public void setLogWriter(PrintWriter logWriter) {
     this.logWriter = logWriter;
   }
 
@@ -123,12 +123,12 @@ public void setLoginTimeout(int timeout) throws SQLException {
   }
 
   @Override
-  public int getLoginTimeout() throws SQLException {
+  public int getLoginTimeout() {
     return 0;
   }
 
   @Override
-  public Logger getParentLogger() throws SQLFeatureNotSupportedException {
+  public Logger getParentLogger() {
     return Logger.getLogger("ArrowFlightJdbc");
   }
 }

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcDriver.java

@@ -19,6 +19,7 @@
 
 import java.io.BufferedReader;
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.Reader;
 import java.net.URI;
@@ -47,6 +48,7 @@
 public class ArrowFlightJdbcDriver extends UnregisteredDriver {
 
   private static final String CONNECT_STRING_PREFIX = "jdbc:arrow-flight://";
+  private static final String CONNECTION_STRING_EXPECTED = "jdbc:arrow-flight://[host][:port][?param1=value&...]";
   private static DriverVersion version;
 
   static {
@@ -81,7 +83,7 @@ public ArrowFlightConnection connect(final String url, final Properties info)
           url,
           properties,
           new RootAllocator(Long.MAX_VALUE));
-    } catch (AssertionError | FlightRuntimeException e) {
+    } catch (final FlightRuntimeException e) {
       throw new SQLException("Failed to connect.", e);
     }
   }
@@ -93,41 +95,39 @@ protected String getFactoryClassName(final JdbcVersion jdbcVersion) {
 
   @Override
   protected DriverVersion createDriverVersion() {
-
-    CreateVersionIfNull:
-    {
-
-      if (version != null) {
-        break CreateVersionIfNull;
+    if (version == null) {
+      final InputStream flightProperties = this.getClass().getResourceAsStream("/properties/flight.properties");
+      if (flightProperties == null) {
+        throw new RuntimeException("Flight Properties not found. Ensure the JAR was built properly.");
       }
-
-      try (Reader reader = new BufferedReader(new InputStreamReader(
-          this.getClass().getResourceAsStream("/properties/flight.properties"),
-          StandardCharsets.UTF_8))) {
+      try (final Reader reader = new BufferedReader(new InputStreamReader(flightProperties, StandardCharsets.UTF_8))) {
         final Properties properties = new Properties();
         properties.load(reader);
 
-        final String parentName = properties
-            .getProperty("org.apache.arrow.flight.name");
-        final String parentVersion = properties
-            .getProperty("org.apache.arrow.flight.version");
+        final String parentName = properties.getProperty("org.apache.arrow.flight.name");
+        final String parentVersion = properties.getProperty("org.apache.arrow.flight.version");
         final String[] pVersion = parentVersion.split("\\.");
 
         final int parentMajorVersion = Integer.parseInt(pVersion[0]);
         final int parentMinorVersion = Integer.parseInt(pVersion[1]);
 
-        final String childName = properties
-            .getProperty("org.apache.arrow.flight.jdbc-driver.name");
-        final String childVersion = properties
-            .getProperty("org.apache.arrow.flight.jdbc-driver.version");
+        final String childName = properties.getProperty("org.apache.arrow.flight.jdbc-driver.name");
+        final String childVersion = properties.getProperty("org.apache.arrow.flight.jdbc-driver.version");
         final String[] cVersion = childVersion.split("\\.");
 
         final int childMajorVersion = Integer.parseInt(cVersion[0]);
         final int childMinorVersion = Integer.parseInt(cVersion[1]);
 
-        version = new DriverVersion(childName, childVersion, parentName,
-            parentVersion, true, childMajorVersion, childMinorVersion,
-            parentMajorVersion, parentMinorVersion);
+        version = new DriverVersion(
+            childName,
+            childVersion,
+            parentName,
+            parentVersion,
+            true,
+            childMajorVersion,
+            childMinorVersion,
+            parentMajorVersion,
+            parentMinorVersion);
       } catch (final IOException e) {
         throw new RuntimeException("Failed to load driver version.", e);
       }
@@ -138,7 +138,7 @@ protected DriverVersion createDriverVersion() {
 
   @Override
   public Meta createMeta(final AvaticaConnection connection) {
-    return new ArrowFlightMetaImpl((ArrowFlightConnection) connection);
+    return new ArrowFlightMetaImpl(connection);
   }
 
   @Override
@@ -147,7 +147,7 @@ protected String getConnectStringPrefix() {
   }
 
   @Override
-  public boolean acceptsURL(final String url) throws SQLException {
+  public boolean acceptsURL(final String url) {
     return Preconditions.checkNotNull(url).startsWith(CONNECT_STRING_PREFIX);
   }
 
@@ -218,7 +218,8 @@ private Map<Object, Object> getUrlsArgs(String url)
      */
 
     if (!url.startsWith("jdbc:")) {
-      throw new SQLException("Malformed/invalid URL!");
+      throw new SQLException("Connection string must start with 'jdbc:'. Expected format: " +
+          CONNECTION_STRING_EXPECTED);
     }
 
     // It's necessary to use a string without "jdbc:" at the beginning to be parsed as a valid URL.
@@ -233,7 +234,8 @@ private Map<Object, Object> getUrlsArgs(String url)
     }
 
     if (!Objects.equals(uri.getScheme(), "arrow-flight")) {
-      throw new SQLException("Malformed/invalid URL!");
+      throw new SQLException("URL Scheme must be 'arrow-flight'. Expected format: " +
+          CONNECTION_STRING_EXPECTED);
     }
 
 
@@ -244,8 +246,7 @@ private Map<Object, Object> getUrlsArgs(String url)
 
     final String extraParams = uri.getRawQuery(); // optional params
 
-    final List<NameValuePair> keyValuePairs =
-        URLEncodedUtils.parse(extraParams, StandardCharsets.UTF_8);
+    final List<NameValuePair> keyValuePairs = URLEncodedUtils.parse(extraParams, StandardCharsets.UTF_8);
     keyValuePairs.forEach(p -> resultMap.put(p.getName(), p.getValue()));
 
     return resultMap;

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcFactory.java

@@ -68,7 +68,7 @@ public AvaticaStatement newStatement(
       final Meta.StatementHandle handle,
       final int resultType,
       final int resultSetConcurrency,
-      final int resultSetHoldability) throws SQLException {
+      final int resultSetHoldability) {
     return new ArrowFlightStatement((ArrowFlightConnection) connection,
         handle, resultType, resultSetConcurrency, resultSetHoldability);
   }
@@ -107,7 +107,7 @@ public AvaticaSpecificDatabaseMetaData newDatabaseMetaData(final AvaticaConnecti
   @Override
   public ResultSetMetaData newResultSetMetaData(
       final AvaticaStatement avaticaStatement,
-      final Meta.Signature signature) throws SQLException {
+      final Meta.Signature signature) {
     return new AvaticaResultSetMetaData(avaticaStatement,
         null, signature);
   }

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcVectorSchemaRootResultSet.java

@@ -89,7 +89,7 @@ public static ArrowFlightJdbcVectorSchemaRootResultSet fromVectorSchemaRoot(
 
   @Override
   protected AvaticaResultSet execute() throws SQLException {
-    throw new RuntimeException();
+    throw new RuntimeException("Can only execute with execute(VectorSchemaRoot)");
   }
 
   void execute(final VectorSchemaRoot vectorSchemaRoot) {

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightMetaImpl.java

@@ -99,8 +99,8 @@ public ExecuteResult execute(final StatementHandle statementHandle,
   @Override
   public ExecuteBatchResult executeBatch(final StatementHandle statementHandle,
                                          final List<List<TypedValue>> parameterValuesList)
-      throws NoSuchStatementException {
-    throw new IllegalStateException();
+      throws IllegalStateException {
+    throw new IllegalStateException("executeBatch not implemented.");
   }
 
   @Override

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/accessor/ArrowFlightJdbcAccessor.java

@@ -17,7 +17,6 @@
 
 package org.apache.arrow.driver.jdbc.accessor;
 
-import static org.apache.arrow.driver.jdbc.utils.ExceptionTemplateThrower.getOperationNotSupported;
 import static org.apache.calcite.avatica.util.Cursor.Accessor;
 
 import java.io.InputStream;
@@ -250,4 +249,8 @@ public <T> T getObject(final Class<T> type) throws SQLException {
     }
     return !type.isPrimitive() && wasNull ? null : type.cast(value);
   }
+
+  private static SQLException getOperationNotSupported(final Class<?> type) {
+    return new SQLException(String.format("Operation not supported for type: %s.", type.getName()));
+  }
 }

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/accessor/ArrowFlightJdbcAccessorFactory.java

@@ -83,7 +83,7 @@
 public class ArrowFlightJdbcAccessorFactory {
 
   /**
-   * Create an accessor according to the its type.
+   * Create an accessor according to its type.
    *
    * @param vector        an instance of an arrow vector.
    * @param getCurrentRow a supplier to check which row is being accessed.

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcDateVectorGetter.java

@@ -35,8 +35,8 @@ private ArrowFlightJdbcDateVectorGetter() {
    * Auxiliary class meant to unify Date*Vector#get implementations with different classes of ValueHolders.
    */
   static class Holder {
-    int isSet;
-    long value;
+    int isSet; // Tells if value is set; 0 = not set, 1 = set
+    long value; // Holds actual value in its respective timeunit
   }
 
   /**

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcIntervalVectorAccessor.java

@@ -92,11 +92,11 @@ public Class<?> getObjectClass() {
 
   @Override
   public String getString() {
-    StringBuilder stringBuilder = this.stringBuilderGetter.get(getCurrentRow());
+    StringBuilder stringBuilder = stringBuilderGetter.get(getCurrentRow());
 
     this.wasNull = stringBuilder == null;
     this.wasNullConsumer.setWasNull(this.wasNull);
-    if (this.wasNull) {
+    if (stringBuilder == null) {
       return null;
     }
 

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorGetter.java

@@ -48,8 +48,8 @@ private ArrowFlightJdbcTimeStampVectorGetter() {
    * Auxiliary class meant to unify TimeStamp*Vector#get implementations with different classes of ValueHolders.
    */
   static class Holder {
-    int isSet;
-    long value;
+    int isSet; // Tells if value is set; 0 = not set, 1 = set
+    long value; // Holds actual value in its respective timeunit
   }
 
   /**

java/flight/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeVectorGetter.java

@@ -39,8 +39,8 @@ private ArrowFlightJdbcTimeVectorGetter() {
    * Auxiliary class meant to unify TimeStamp*Vector#get implementations with different classes of ValueHolders.
    */
   static class Holder {
-    int isSet;
-    long value;
+    int isSet; // Tells if value is set; 0 = not set, 1 = set
+    long value; // Holds actual value in its respective timeunit
   }
 
   /**