GH-33475: [Java] Add parameter binding for Prepared Statements in JDBC driver #38404

aiguofer · 2023-10-23T15:33:58Z

This PR is a combination of #33961 and #14627. The goal is to support parametrized queries through the Arrow Flight SQL JDBC driver.

An Arrow Flight SQL server returns a Schema for the PreparedStatement parameters. The driver then converts the Field list associated with the Schema into a list of AvaticaParameter. When the user sets values for the parameters, Avatica generates a list of TypedValue, which we then bind to each parameter vector. This conversion between Arrow and Avatica is handled by implementations of a AvaticaParameterConverter interface for each Arrow type. This interface which provides 2 methods:

createParameter: Create an AvaticaParameter from the given Arrow Field.
bindParameter: Cast the given TypedValue and bind it to the FieldVector at the specified index.

This PR purposely leaves out a few features:

We currently naively cast the TypedValue values assuming users set the type correctly. If this cast fails, we raise an exception letting the user know that the cast is not supported. This could be improved in subsequent PRs to do smarter conversions from other types.
We currently don't provide conversions for complex types such as List, Map, Struct, Union, Interval, and Duration. The stubs are there so they can be implemented as needed.
Tests for specific types have not been implemented. I'm not very familiar with a lot of these JDBC types so it's hard to implement rigorous tets.

github-actions · 2023-10-23T15:34:32Z

⚠️ GitHub issue #33475 has been automatically assigned in GitHub to PR creator.

github-actions · 2023-10-23T15:39:46Z

⚠️ GitHub issue #33475 has been automatically assigned in GitHub to PR creator.

github-actions · 2023-10-23T19:54:31Z

⚠️ GitHub issue #33475 has been automatically assigned in GitHub to PR creator.

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

...t-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightPreparedStatement.java

.../flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowToJdbcUtils.java

...ht-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/MockFlightSqlProducer.java

.../flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowToJdbcUtils.java

.../flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/TypedValueBinder.java

...l-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ArrowFlightPreparedStatementTest.java

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java

github-actions · 2023-10-30T22:20:26Z

⚠️ GitHub issue #33475 has been automatically assigned in GitHub to PR creator.

aiguofer · 2023-10-30T22:34:34Z

I refactored this quite a bit to put Arrow <-> Avatica conversions in the same class for each Arrow type. This should help ensure we do this correctly for all Arrow types.

Currently, testing is pretty basic and we're missing conversions for complex types, but I think it's a pretty good starting point to implement the rest.

I'm not sure why the one test failed and can't figure out how to re-run it.

...src/main/java/org/apache/arrow/driver/jdbc/converter/impl/DateAvaticaParameterConverter.java

aiguofer · 2023-11-03T18:37:31Z

@lidavidm @jduo Hey all, I've spent a lot of time on this PR. It's a little frustrating that a PR that started well after mine has merged and now I have to deal with merge conflicts while mine still hasn't been thoroughly reviewed. We need this for one of our partner integrations and there's still been no movement on this. Could I at least get some eyes on this?

lidavidm · 2023-11-03T19:14:14Z

@aiguofer sorry about that - I'll give this a review over the weekend once Github is working again.

jduo · 2023-11-03T19:38:48Z

@lidavidm @jduo Hey all, I've spent a lot of time on this PR. It's a little frustrating that a PR that started well after mine has merged and now I have to deal with merge conflicts while mine still hasn't been thoroughly reviewed. We need this for one of our partner integrations and there's still been no movement on this. Could I at least get some eyes on this?

Hi @aiguofer . It looks good. I didn't have additional comments.

lidavidm

None of my comments are critical - if you want to just file an issue to track them that would be fine

We currently naively cast the TypedValue values assuming users set the type correctly. If this cast fails, we raise an exception letting the user know that the cast is not supported. This could be improved in subsequent PRs to do smarter conversions from other types.

We can file a followup for this too.

We currently don't provide conversions for complex types such as List, Map, Struct, Union, Interval, and Duration. The stubs are there so they can be implemented as needed.

Ditto.

Tests for specific types have not been implemented. I'm not very familiar with a lot of these JDBC types so it's hard to implement rigorous tets.

I would be in favor of trying to set up integration suites with systems that depend on the driver, in the same way we have integration tests with Spark and Pandas (two other projects that heavily depend on Arrow). But this is also something we can defer.

lidavidm · 2023-11-03T19:30:00Z

...dbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/AvaticaParameterConverter.java

+   *
+   * @param vector FieldVector that the parameter should be bound to.
+   * @param typedValue TypedValue to bind as a parameter.
+   * @param index Vector index that the TypedValue should be bound to.


0-index, right? (I ask because JDBC is often 1-indexed)

Correct! that would be the index on the Arrow vector. I'll add to the docstring just to be more clear

lidavidm · 2023-11-03T19:41:10Z

...c/main/java/org/apache/arrow/driver/jdbc/converter/impl/BinaryAvaticaParameterConverter.java

+
+  @Override
+  public boolean bindParameter(FieldVector vector, TypedValue typedValue, int index) {
+    byte[] value = (byte[]) typedValue.toJdbc(null);


Do we need to handle value == null? Or in general how does Avatica handle null bind parameters?

Ohhh good catch. Avatica doesn't really do anything special for null values so it just returns null.

For example:

public Object toJdbc(Calendar calendar) { return this.value == null ? null : serialToJdbc(this.type, this.componentType, this.value, calendar); }

I guess we need to call setNull if the value is null?

Yeah - do you want to handle that here, or do you want to file a follow-up for now?

Actually, let's just get this in before something else conflicts with it

lidavidm · 2023-11-03T19:53:54Z

...ain/java/org/apache/arrow/driver/jdbc/converter/impl/TimestampAvaticaParameterConverter.java

+
+  @Override
+  public boolean bindParameter(FieldVector vector, TypedValue typedValue, int index) {
+    // FIXME: how should we handle TZ? Do we need to convert the value to the TZ on the vector?


The timezone on the vector is purely for display; the underlying value is always UTC in Arrow.

cool that's what I was thinking. At this point Avatica has already converted to UTC so that's perfect.

lidavidm · 2023-11-03T19:56:48Z

...t-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/AvaticaParameterBinder.java

+  private void bind(FieldVector vector, TypedValue typedValue, int index) {
+    try {
+      if (!vector.getField().getType().accept(new BinderVisitor(vector, typedValue, index))) {
+        throw new RuntimeException(


nit: is there a more appropriate exception than RuntimeException?

I was thinking some type of NotImplementedException would be ideal but I couldn't find an exception like that. Any thoughts on an alternative? UnsupportedOperationException seem to imply that it will not be supported, which might not be what we want.

IMO UnsupportedOperationException is fine for this sort of thing

lidavidm · 2023-11-04T17:06:00Z

I filed a general followups issue at #38585

Thanks again @aiguofer!

aiguofer · 2023-11-04T17:36:09Z

I filed a general followups issue at #38585

Thanks again @aiguofer!

Perfect sounds good! I can work on a few of those soon after this!

conbench-apache-arrow · 2023-11-04T20:25:56Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit fc8c6b7.

There was 1 benchmark result with an error:

Commit Run on ec2-m5-4xlarge-us-east-2 at 2023-11-04 18:15:17Z
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-15, scale_factor=10

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

…in JDBC driver (apache#38404) This PR is a combination of apache#33961 and apache#14627. The goal is to support parametrized queries through the Arrow Flight SQL JDBC driver. An Arrow Flight SQL server returns a Schema for the `PreparedStatement` parameters. The driver then converts the `Field` list associated with the Schema into a list of `AvaticaParameter`. When the user sets values for the parameters, Avatica generates a list of `TypedValue`, which we then bind to each parameter vector. This conversion between Arrow and Avatica is handled by implementations of a `AvaticaParameterConverter` interface for each Arrow type. This interface which provides 2 methods: - createParameter: Create an `AvaticaParameter` from the given Arrow `Field`. - bindParameter: Cast the given `TypedValue` and bind it to the `FieldVector` at the specified index. This PR purposely leaves out a few features: - We currently naively cast the `TypedValue` values assuming users set the type correctly. If this cast fails, we raise an exception letting the user know that the cast is not supported. This could be improved in subsequent PRs to do smarter conversions from other types. - We currently don't provide conversions for complex types such as List, Map, Struct, Union, Interval, and Duration. The stubs are there so they can be implemented as needed. - Tests for specific types have not been implemented. I'm not very familiar with a lot of these JDBC types so it's hard to implement rigorous tets. * Closes: apache#33475 * Closes: apache#35536 Authored-by: Diego Fernandez <[email protected]> Signed-off-by: David Li <[email protected]>

Add parameter binding for Prepared Statements

ef349c6

aiguofer requested a review from lidavidm as a code owner October 23, 2023 15:33

aiguofer marked this pull request as draft October 23, 2023 15:34

github-actions bot added Component: Java awaiting review Awaiting review labels Oct 23, 2023

aiguofer changed the title ~~GH-33475: [Java][jdbc-driver] Add parameter binding for Prepared Statements~~ GH-33475: [Java] Add parameter binding for Prepared Statements in JDBC driver Oct 23, 2023

aiguofer added 2 commits October 23, 2023 18:55

Cleanup, fix parameter VSR closing, add basic test

0049dad

fix docstring

4d0ce8f

aiguofer commented Oct 23, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 23, 2023

Add test for binding query params

d2cb667

aiguofer commented Oct 23, 2023

View reviewed changes

...ht-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/utils/MockFlightSqlProducer.java Show resolved Hide resolved

aiguofer added 2 commits October 23, 2023 22:04

Fix test for query params

cad225a

.

3bb139b

aiguofer marked this pull request as ready for review October 23, 2023 22:07

modify for consistency

a30f0af