This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git
The following commit(s) were added to refs/heads/main by this push:
new e69723a fix(java/driver/jdbc): clean up buffer leaks (#533)
e69723a is described below
commit e69723a158276d973cf6b84127ea590844e1efb5
Author: David Li <[email protected]>
AuthorDate: Wed Mar 29 13:24:27 2023 -0400
fix(java/driver/jdbc): clean up buffer leaks (#533)
* Close the allocator in JdbcConnection.
* Allow supplying an allocator to JdbcDriver.
* Fix miscellaneous memory leaks discovered from those changes.
Fixes #476.
Fixes #477.
---
.../adbc/driver/flightsql/FlightSqlQuirks.java | 2 +-
.../arrow/adbc/driver/jdbc/derby/DerbyQuirks.java | 5 +++--
.../driver/jdbc/postgresql/PostgresqlQuirks.java | 5 +++--
.../arrow/adbc/driver/jdbc/JdbcArrowReader.java | 12 ++++++-----
.../arrow/adbc/driver/jdbc/JdbcBindReader.java | 19 ++++++++++--------
.../arrow/adbc/driver/jdbc/JdbcConnection.java | 10 +++++++++-
.../apache/arrow/adbc/driver/jdbc/JdbcDriver.java | 18 ++++++++++++-----
.../testsuite/AbstractConnectionMetadataTest.java | 4 ++--
.../driver/testsuite/AbstractConnectionTest.java | 8 ++++++--
.../testsuite/AbstractPartitionDescriptorTest.java | 4 ++--
.../driver/testsuite/AbstractStatementTest.java | 23 ++++++++++++++++++++--
.../driver/testsuite/AbstractTransactionTest.java | 4 ++--
.../adbc/driver/testsuite/SqlValidationQuirks.java | 3 ++-
java/pom.xml | 2 +-
14 files changed, 83 insertions(+), 36 deletions(-)
diff --git
a/java/driver/flight-sql-validation/src/test/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlQuirks.java
b/java/driver/flight-sql-validation/src/test/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlQuirks.java
index d2cd614..2f1840d 100644
---
a/java/driver/flight-sql-validation/src/test/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlQuirks.java
+++
b/java/driver/flight-sql-validation/src/test/java/org/apache/arrow/adbc/driver/flightsql/FlightSqlQuirks.java
@@ -43,7 +43,7 @@ public class FlightSqlQuirks extends SqlValidationQuirks {
}
@Override
- public AdbcDatabase initDatabase() throws AdbcException {
+ public AdbcDatabase initDatabase(BufferAllocator allocator) throws
AdbcException {
String url = getFlightLocation();
final Map<String, Object> parameters = new HashMap<>();
diff --git
a/java/driver/jdbc-validation-derby/src/test/java/org/apache/arrow/adbc/driver/jdbc/derby/DerbyQuirks.java
b/java/driver/jdbc-validation-derby/src/test/java/org/apache/arrow/adbc/driver/jdbc/derby/DerbyQuirks.java
index b8a4def..1df0398 100644
---
a/java/driver/jdbc-validation-derby/src/test/java/org/apache/arrow/adbc/driver/jdbc/derby/DerbyQuirks.java
+++
b/java/driver/jdbc-validation-derby/src/test/java/org/apache/arrow/adbc/driver/jdbc/derby/DerbyQuirks.java
@@ -29,6 +29,7 @@ import org.apache.arrow.adbc.core.AdbcDriver;
import org.apache.arrow.adbc.core.AdbcException;
import org.apache.arrow.adbc.driver.jdbc.JdbcDriver;
import org.apache.arrow.adbc.driver.testsuite.SqlValidationQuirks;
+import org.apache.arrow.memory.BufferAllocator;
public class DerbyQuirks extends SqlValidationQuirks {
private final String jdbcUrl;
@@ -38,10 +39,10 @@ public class DerbyQuirks extends SqlValidationQuirks {
}
@Override
- public AdbcDatabase initDatabase() throws AdbcException {
+ public AdbcDatabase initDatabase(BufferAllocator allocator) throws
AdbcException {
final Map<String, Object> parameters = new HashMap<>();
parameters.put(AdbcDriver.PARAM_URL, jdbcUrl);
- return JdbcDriver.INSTANCE.open(parameters);
+ return new JdbcDriver(allocator).open(parameters);
}
@Override
diff --git
a/java/driver/jdbc-validation-postgresql/src/test/java/org/apache/arrow/adbc/driver/jdbc/postgresql/PostgresqlQuirks.java
b/java/driver/jdbc-validation-postgresql/src/test/java/org/apache/arrow/adbc/driver/jdbc/postgresql/PostgresqlQuirks.java
index c11f5d1..7018d75 100644
---
a/java/driver/jdbc-validation-postgresql/src/test/java/org/apache/arrow/adbc/driver/jdbc/postgresql/PostgresqlQuirks.java
+++
b/java/driver/jdbc-validation-postgresql/src/test/java/org/apache/arrow/adbc/driver/jdbc/postgresql/PostgresqlQuirks.java
@@ -29,6 +29,7 @@ import org.apache.arrow.adbc.core.AdbcException;
import org.apache.arrow.adbc.driver.jdbc.JdbcDriver;
import org.apache.arrow.adbc.driver.testsuite.SqlValidationQuirks;
import org.apache.arrow.adbc.sql.SqlQuirks;
+import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.vector.types.pojo.ArrowType;
import org.junit.jupiter.api.Assumptions;
@@ -49,7 +50,7 @@ public class PostgresqlQuirks extends SqlValidationQuirks {
}
@Override
- public AdbcDatabase initDatabase() throws AdbcException {
+ public AdbcDatabase initDatabase(BufferAllocator allocator) throws
AdbcException {
String url = makeJdbcUrl();
final Map<String, Object> parameters = new HashMap<>();
@@ -65,7 +66,7 @@ public class PostgresqlQuirks extends SqlValidationQuirks {
return
SqlQuirks.DEFAULT_ARROW_TYPE_TO_SQL_TYPE_NAME_MAPPING.apply(arrowType);
}))
.build());
- return JdbcDriver.INSTANCE.open(parameters);
+ return new JdbcDriver(allocator).open(parameters);
}
@Override
diff --git
a/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcArrowReader.java
b/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcArrowReader.java
index 4445729..1ddbf1c 100644
---
a/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcArrowReader.java
+++
b/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcArrowReader.java
@@ -78,11 +78,13 @@ public class JdbcArrowReader extends ArrowReader {
@Override
public boolean loadNextBatch() {
if (!delegate.hasNext()) return false;
- final VectorSchemaRoot root = delegate.next();
- final VectorUnloader unloader = new VectorUnloader(root);
- final ArrowRecordBatch recordBatch = unloader.getRecordBatch();
- bytesRead += recordBatch.computeBodyLength();
- loadRecordBatch(recordBatch);
+ try (final VectorSchemaRoot root = delegate.next()) {
+ final VectorUnloader unloader = new VectorUnloader(root);
+ try (final ArrowRecordBatch recordBatch = unloader.getRecordBatch()) {
+ bytesRead += recordBatch.computeBodyLength();
+ loadRecordBatch(recordBatch);
+ }
+ }
return true;
}
diff --git
a/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcBindReader.java
b/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcBindReader.java
index 9b314d5..167d3f2 100644
---
a/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcBindReader.java
+++
b/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcBindReader.java
@@ -54,9 +54,10 @@ final class JdbcBindReader extends ArrowReader {
}
}
- final VectorSchemaRoot root = currentSource.next();
- try (final ArrowRecordBatch batch = new
VectorUnloader(root).getRecordBatch()) {
- loadRecordBatch(batch);
+ try (final VectorSchemaRoot root = currentSource.next()) {
+ try (final ArrowRecordBatch batch = new
VectorUnloader(root).getRecordBatch()) {
+ loadRecordBatch(batch);
+ }
}
return true;
}
@@ -68,11 +69,13 @@ final class JdbcBindReader extends ArrowReader {
@Override
protected void closeReadSource() throws IOException {
- try {
- // Do not close PreparedStatement so we can reuse it
- currentResultSet.close();
- } catch (SQLException e) {
- throw new IOException(e);
+ if (currentResultSet != null) {
+ try {
+ // Do not close PreparedStatement so we can reuse it
+ currentResultSet.close();
+ } catch (SQLException e) {
+ throw new IOException(e);
+ }
}
}
diff --git
a/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcConnection.java
b/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcConnection.java
index e5f4a6a..205f193 100644
---
a/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcConnection.java
+++
b/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcConnection.java
@@ -32,6 +32,7 @@ import org.apache.arrow.adbc.core.IsolationLevel;
import org.apache.arrow.adbc.core.StandardSchemas;
import org.apache.arrow.adbc.sql.SqlQuirks;
import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.util.AutoCloseables;
import org.apache.arrow.vector.VectorSchemaRoot;
import org.apache.arrow.vector.ipc.ArrowReader;
import org.apache.arrow.vector.types.pojo.ArrowType;
@@ -44,6 +45,13 @@ public class JdbcConnection implements AdbcConnection {
private final Connection connection;
private final SqlQuirks quirks;
+ /**
+ * Create a new connection.
+ *
+ * @param allocator The allocator to use. The connection will close the
allocator when done.
+ * @param connection The JDBC connection.
+ * @param quirks Backend-specific quirks to account for.
+ */
JdbcConnection(BufferAllocator allocator, Connection connection, SqlQuirks
quirks) {
this.allocator = allocator;
this.connection = connection;
@@ -253,7 +261,7 @@ public class JdbcConnection implements AdbcConnection {
@Override
public void close() throws Exception {
- connection.close();
+ AutoCloseables.close(connection, allocator);
}
private void checkAutoCommit() throws AdbcException, SQLException {
diff --git
a/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcDriver.java
b/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcDriver.java
index 67db75a..7925acb 100644
---
a/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcDriver.java
+++
b/java/driver/jdbc/src/main/java/org/apache/arrow/adbc/driver/jdbc/JdbcDriver.java
@@ -17,6 +17,7 @@
package org.apache.arrow.adbc.driver.jdbc;
import java.util.Map;
+import java.util.Objects;
import org.apache.arrow.adbc.core.AdbcDatabase;
import org.apache.arrow.adbc.core.AdbcDriver;
import org.apache.arrow.adbc.core.AdbcException;
@@ -27,14 +28,21 @@ import org.apache.arrow.memory.RootAllocator;
import org.apache.arrow.util.Preconditions;
/** An ADBC driver wrapping the JDBC API. */
-public enum JdbcDriver implements AdbcDriver {
- INSTANCE;
+public class JdbcDriver implements AdbcDriver {
+ public static final JdbcDriver INSTANCE = new JdbcDriver();
+
+ static {
+
AdbcDriverManager.getInstance().registerDriver("org.apache.arrow.adbc.driver.jdbc",
INSTANCE);
+ }
private final BufferAllocator allocator;
- JdbcDriver() {
- allocator = new RootAllocator();
-
AdbcDriverManager.getInstance().registerDriver("org.apache.arrow.adbc.driver.jdbc",
this);
+ public JdbcDriver() {
+ this(new RootAllocator());
+ }
+
+ public JdbcDriver(BufferAllocator allocator) {
+ this.allocator = Objects.requireNonNull(allocator);
}
@Override
diff --git
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractConnectionMetadataTest.java
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractConnectionMetadataTest.java
index 5514097..5d14339 100644
---
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractConnectionMetadataTest.java
+++
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractConnectionMetadataTest.java
@@ -68,9 +68,9 @@ public abstract class AbstractConnectionMetadataTest {
@BeforeEach
public void beforeEach() throws Exception {
Preconditions.checkNotNull(quirks, "Must initialize quirks in subclass
with @BeforeAll");
- database = quirks.initDatabase();
- connection = database.connect();
allocator = new RootAllocator();
+ database = quirks.initDatabase(allocator);
+ connection = database.connect();
util = new SqlTestUtil(quirks);
tableName = quirks.caseFoldTableName("foo");
mainTable = quirks.caseFoldTableName("product");
diff --git
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractConnectionTest.java
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractConnectionTest.java
index 80efac2..9915636 100644
---
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractConnectionTest.java
+++
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractConnectionTest.java
@@ -21,6 +21,8 @@ import static org.assertj.core.api.Assertions.assertThat;
import org.apache.arrow.adbc.core.AdbcConnection;
import org.apache.arrow.adbc.core.AdbcDatabase;
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
import org.apache.arrow.util.AutoCloseables;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
@@ -30,18 +32,20 @@ public abstract class AbstractConnectionTest {
/** Must be initialized by the subclass. */
protected static SqlValidationQuirks quirks;
+ protected BufferAllocator allocator;
protected AdbcDatabase database;
protected AdbcConnection connection;
@BeforeEach
public void beforeEach() throws Exception {
- database = quirks.initDatabase();
+ allocator = new RootAllocator();
+ database = quirks.initDatabase(allocator);
connection = database.connect();
}
@AfterEach
public void afterEach() throws Exception {
- AutoCloseables.close(connection, database);
+ AutoCloseables.close(connection, database, allocator);
}
@Test
diff --git
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractPartitionDescriptorTest.java
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractPartitionDescriptorTest.java
index 8a1dfb9..a0dee52 100644
---
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractPartitionDescriptorTest.java
+++
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractPartitionDescriptorTest.java
@@ -55,9 +55,9 @@ public abstract class AbstractPartitionDescriptorTest {
@BeforeEach
public void beforeEach() throws Exception {
Preconditions.checkNotNull(quirks, "Must initialize quirks in subclass
with @BeforeAll");
- database = quirks.initDatabase();
- connection = database.connect();
allocator = new RootAllocator();
+ database = quirks.initDatabase(allocator);
+ connection = database.connect();
util = new SqlTestUtil(quirks);
tableName = quirks.caseFoldTableName("bulktable");
schema =
diff --git
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractStatementTest.java
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractStatementTest.java
index 4b8dd14..b264bf4 100644
---
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractStatementTest.java
+++
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractStatementTest.java
@@ -59,9 +59,9 @@ public abstract class AbstractStatementTest {
@BeforeEach
public void beforeEach() throws Exception {
Preconditions.checkNotNull(quirks, "Must initialize quirks in subclass
with @BeforeAll");
- database = quirks.initDatabase();
- connection = database.connect();
allocator = new RootAllocator();
+ database = quirks.initDatabase(allocator);
+ connection = database.connect();
util = new SqlTestUtil(quirks);
tableName = quirks.caseFoldTableName("bulktable");
schema =
@@ -203,6 +203,25 @@ public abstract class AbstractStatementTest {
}
}
+ @Test
+ public void prepareQueryWithParametersNoOp() throws Exception {
+ final Schema expectedSchema = util.ingestTableIntsStrs(allocator,
connection, tableName);
+ final Schema paramsSchema =
+ new
Schema(Collections.singletonList(expectedSchema.getFields().get(0)));
+ try (final AdbcStatement stmt = connection.createStatement();
+ final VectorSchemaRoot params = VectorSchemaRoot.create(paramsSchema,
allocator)) {
+ stmt.setSqlQuery(String.format("SELECT * FROM %s WHERE INTS = ?",
tableName));
+ stmt.prepare();
+ stmt.bind(params);
+ IntVector param0 = (IntVector) params.getVector(0);
+ param0.setSafe(0, 1);
+ param0.setSafe(1, 2);
+ params.setRowCount(2);
+ // Ensure things still close properly if we never actually drain the
result
+ stmt.executeQuery().close();
+ }
+ }
+
@Test
public void prepareQueryWithParameters() throws Exception {
final Schema expectedSchema = util.ingestTableIntsStrs(allocator,
connection, tableName);
diff --git
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractTransactionTest.java
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractTransactionTest.java
index 230afc9..29265ba 100644
---
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractTransactionTest.java
+++
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/AbstractTransactionTest.java
@@ -52,9 +52,9 @@ public abstract class AbstractTransactionTest {
@BeforeEach
public void beforeEach() throws Exception {
Preconditions.checkNotNull(quirks, "Must initialize quirks in subclass
with @BeforeAll");
- database = quirks.initDatabase();
- connection = database.connect();
allocator = new RootAllocator();
+ database = quirks.initDatabase(allocator);
+ connection = database.connect();
}
@AfterEach
diff --git
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/SqlValidationQuirks.java
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/SqlValidationQuirks.java
index ee60e8b..6126972 100644
---
a/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/SqlValidationQuirks.java
+++
b/java/driver/validation/src/main/java/org/apache/arrow/adbc/driver/testsuite/SqlValidationQuirks.java
@@ -21,10 +21,11 @@ import java.util.List;
import java.util.stream.Collectors;
import org.apache.arrow.adbc.core.AdbcDatabase;
import org.apache.arrow.adbc.core.AdbcException;
+import org.apache.arrow.memory.BufferAllocator;
/** Account for driver/vendor-specific quirks in implementing validation
tests. */
public abstract class SqlValidationQuirks {
- public abstract AdbcDatabase initDatabase() throws AdbcException;
+ public abstract AdbcDatabase initDatabase(BufferAllocator allocator) throws
AdbcException;
public void cleanupTable(String name) throws Exception {}
diff --git a/java/pom.xml b/java/pom.xml
index 29abf76..bb983ad 100644
--- a/java/pom.xml
+++ b/java/pom.xml
@@ -28,7 +28,7 @@
<url>https://arrow.apache.org/</url>
<properties>
- <dep.arrow.version>10.0.0</dep.arrow.version>
+ <dep.arrow.version>11.0.0</dep.arrow.version>
<adbc.version>0.4.0-SNAPSHOT</adbc.version>
</properties>
