Copilot commented on code in PR #4359:
URL: https://github.com/apache/arrow-adbc/pull/4359#discussion_r3353388181
##########
java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/JniStatement.java:
##########
@@ -53,23 +59,48 @@ public void setSqlQuery(String query) throws AdbcException {
@Override
public void bind(VectorSchemaRoot root) throws AdbcException {
+ clearBind();
this.bindRoot = root;
}
+ @Override
+ public void bind(ArrowReader reader) throws AdbcException {
+ clearBind();
+ this.bindRoot = null;
+ this.bindStream = reader;
+ }
+
+ private void clearBind() throws AdbcException {
+ if (this.bindStream != null) {
+ try {
+ this.bindStream.close();
+ } catch (IOException e) {
+ throw AdbcException.internal("[jni] failed to close previous bind
stream").withCause(e);
+ } finally {
+ this.bindStream = null;
+ }
+ }
+ this.bindRoot = null;
+ }
+
// The C Data export takes ownership of the data at bind time and ignores
subsequent
// client changes to the bound root. Defer the export until execution so we
capture
// the final state of the VectorSchemaRoot.
private void exportBind() throws AdbcException {
- if (bindRoot == null) {
- return;
- }
- try (final ArrowArray batch = ArrowArray.allocateNew(allocator);
- final ArrowSchema schema = ArrowSchema.allocateNew(allocator)) {
- // TODO(lidavidm): we may need a way to separately provide a dictionary
provider
- Data.exportSchema(allocator, bindRoot.getSchema(), null, schema);
- Data.exportVectorSchemaRoot(allocator, bindRoot, null, batch);
-
- JniLoader.INSTANCE.statementBind(handle, batch, schema);
+ if (bindRoot != null) {
+ try (final ArrowArray batch = ArrowArray.allocateNew(allocator);
+ final ArrowSchema schema = ArrowSchema.allocateNew(allocator)) {
+ // TODO(lidavidm): we may need a way to separately provide a
dictionary provider
+ Data.exportSchema(allocator, bindRoot.getSchema(), null, schema);
+ Data.exportVectorSchemaRoot(allocator, bindRoot, null, batch);
+
+ JniLoader.INSTANCE.statementBind(handle, batch, schema);
+ }
+ } else if (bindStream != null) {
+ try (final ArrowArrayStream stream =
ArrowArrayStream.allocateNew(allocator)) {
+ Data.exportArrayStream(allocator, bindStream, stream);
+ JniLoader.INSTANCE.statementBindStream(handle, stream);
+ }
Review Comment:
After exporting/binding an ArrowReader via AdbcStatementBindStream, the C
driver takes ownership of the exported ArrowArrayStream and will invoke its
release callback (which will close the underlying ArrowReader). Keeping
`bindStream` set here means `clearBind()`/`close()` may later close the same
reader again, or close it before the native statement releases it, violating
the ownership contract of `AdbcStatementBindStream` (see adbc.h:2114-2116).
Clear the Java reference once the bind succeeds so the statement no longer
attempts to manage the reader lifecycle after ownership is transferred.
##########
java/driver/jni-validation/src/test/java/org/apache/arrow/adbc/driver/jni/PostgresIntegrationTest.java:
##########
@@ -569,6 +573,37 @@ void bindUpdateQuery() throws Exception {
}
}
+ @Test
+ void bindStream() throws Exception {
+ // Create temp Arrow file to get an ArrowReader
+ Schema schema = new Schema(List.of(Field.nullable("i",
Types.MinorType.INT.getType())));
Review Comment:
For stream binding, other tests in this file use parameter-style field names
(e.g. "$1" in bindSelectQuery). If this test is updated to actually execute a
statement (which it should), the stream schema should match that convention so
the driver can map columns to bind parameters consistently.
##########
java/driver/jni-validation/src/test/java/org/apache/arrow/adbc/driver/jni/PostgresIntegrationTest.java:
##########
@@ -569,6 +573,37 @@ void bindUpdateQuery() throws Exception {
}
}
+ @Test
+ void bindStream() throws Exception {
+ // Create temp Arrow file to get an ArrowReader
+ Schema schema = new Schema(List.of(Field.nullable("i",
Types.MinorType.INT.getType())));
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (VectorSchemaRoot root = VectorSchemaRoot.create(schema, allocator);
+ ArrowStreamWriter writer = new ArrowStreamWriter(root, null, baos)) {
+ writer.start();
+
+ IntVector iv = (IntVector) root.getVector(0);
+
+ iv.setSafe(0, 1);
+ iv.setSafe(1, 42);
+ iv.setNull(2);
+ root.setRowCount(3);
+ writer.writeBatch();
+
+ iv.setSafe(0, 10);
+ iv.setSafe(1, 20);
+ iv.setSafe(2, 30);
+ root.setRowCount(3);
+ writer.writeBatch();
+ }
+
+ ArrowStreamReader reader =
+ new ArrowStreamReader(new ByteArrayInputStream(baos.toByteArray()),
allocator);
+ try (var stmt = conn.createStatement()) {
+ stmt.bind(reader);
+ }
Review Comment:
This test currently never executes a query/update after binding the
ArrowReader, so it doesn't exercise the new `statementBindStream` JNI path
(exporting the ArrowReader to C and letting the driver consume it). Execute a
simple bound query and assert results so regressions in stream binding are
caught.
##########
java/driver/jni-validation/src/test/java/org/apache/arrow/adbc/driver/jni/SqlServerIntegrationTest.java:
##########
@@ -598,6 +602,37 @@ void bindUpdateQuery() throws Exception {
}
}
+ @Test
+ void bindStream() throws Exception {
+ // Create temp Arrow file to get an ArrowReader
+ Schema schema = new Schema(List.of(Field.nullable("i",
Types.MinorType.INT.getType())));
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (VectorSchemaRoot root = VectorSchemaRoot.create(schema, allocator);
+ ArrowStreamWriter writer = new ArrowStreamWriter(root, null, baos)) {
+ writer.start();
+
+ IntVector iv = (IntVector) root.getVector(0);
+
+ iv.setSafe(0, 1);
+ iv.setSafe(1, 42);
+ iv.setNull(2);
+ root.setRowCount(3);
+ writer.writeBatch();
+
+ iv.setSafe(0, 10);
+ iv.setSafe(1, 20);
+ iv.setSafe(2, 30);
+ root.setRowCount(3);
+ writer.writeBatch();
+ }
+
+ ArrowStreamReader reader =
+ new ArrowStreamReader(new ByteArrayInputStream(baos.toByteArray()),
allocator);
+ try (var stmt = conn.createStatement()) {
+ stmt.bind(reader);
+ }
Review Comment:
This test only binds the ArrowReader and closes the statement; it never
executes anything, so it doesn't exercise the new JNI stream-binding/export
path. Execute a simple bound query (similar to bindSelectQuery) and assert the
results so stream binding failures are caught.
##########
java/driver/jni-validation/src/test/java/org/apache/arrow/adbc/driver/jni/SqlServerIntegrationTest.java:
##########
@@ -598,6 +602,37 @@ void bindUpdateQuery() throws Exception {
}
}
+ @Test
+ void bindStream() throws Exception {
+ // Create temp Arrow file to get an ArrowReader
+ Schema schema = new Schema(List.of(Field.nullable("i",
Types.MinorType.INT.getType())));
Review Comment:
To meaningfully test stream binding (i.e., execute a statement using the
bound values), the stream schema should follow the parameter-style naming used
elsewhere in this file (e.g., "$1" in bindSelectQuery) so the driver can map
stream columns to bind parameters consistently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
