GH-33475: [Java][FlightRPC] Send prepared statement parameters #33961

avantgardnerio · 2023-01-31T21:02:15Z

Rationale for this change

Presently, the flight-sql-jdbc-driver does not send prepared statement parameter values to the server. This prevents Arrow FlightSql server implementors for receiving these values and users from being able to use prepared statements properly. This causes some significant issues, in our case not being able to performance test our database with TPC-C.

Will resolve: #33475

What changes are included in this PR?

Parsing and saving the schema of statement parameters sent by the server
(Hopefully soon) sending parameter values back to the server.

Are these changes tested?

Currently, only by an integration test against our proprietary, unreleased database.

Are there any user-facing changes?

Parameterized queries should work when this PR is completed.

Closes: [Java][FlightRPC] FlightSQL error: 'Parameter ordinal out of range' executing a prepared stmt with params #33475

github-actions · 2023-01-31T21:02:38Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

lidavidm · 2023-01-31T21:05:56Z

Oh cool, you worked this out. I had half a PR but never finished it.

avantgardnerio · 2023-01-31T21:06:23Z

...ht/flight-sql-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/utils/ConvertUtils.java

+    return false;
+  }
+
+  static int toJdbcType(ArrowType.ArrowTypeID type) {


I am uncertain where these methods should go.

avantgardnerio · 2023-01-31T21:07:06Z

...ht/flight-sql-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/utils/ConvertUtils.java

+      case Duration:
+        return Timestamp.class.getCanonicalName();
+    }
+    return null;


FlightSql schema are quite the superset of JDBC, so the mapping of deeply nested structs and such should probably be a subsequent PR?

lidavidm · 2023-01-31T21:07:25Z

java/vector/src/test/java/org/apache/arrow/vector/table/RowTest.java

- * limitations under the License.
- */
-
-package org.apache.arrow.vector.table;


Did you mean to delete this file?

Temporarily yes. I could not get it to compile and will put it back before I un-draft.

avantgardnerio · 2023-01-31T21:07:52Z

...ht-sql-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcDriverTest.java

+      try(PreparedStatement ps = con.prepareStatement("select * from person where id=$1")) {
+        ParameterMetaData md = ps.getParameterMetaData();
+        assertEquals(1, md.getParameterCount());
+        assertEquals("Int", md.getParameterTypeName(1));


These assertions pass against our server implementation, which is part #1 of this PR.

avantgardnerio · 2023-01-31T21:09:03Z

...ht-sql-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcDriverTest.java

+        ParameterMetaData md = ps.getParameterMetaData();
+        assertEquals(1, md.getParameterCount());
+        assertEquals("Int", md.getParameterTypeName(1));
+        ps.setInt(1, 1);


This does not seem to affect parameterBindingRoot so the parameters are not sent back to the server.

I am a bit lost and would appreciate some help following the flow of how those parameters should be sent and why they are not going through at present.

lidavidm · 2023-01-31T21:09:20Z

In general you can look at the arrow-jdbc module for existing mapping between Arrow and JDBC types, as you've noted, not all types are easily handled

avantgardnerio · 2023-01-31T21:09:34Z

java/vector/src/test/java/org/apache/arrow/vector/table/RowTest.java

@@ -1,856 +0,0 @@
-/*


I'll put this back before I un-draft the PR. I could not get it to compile locally, so I deleted it for now.

avantgardnerio · 2023-01-31T21:13:22Z

...ht-sql-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcDriverTest.java

@@ -133,6 +126,28 @@ public void testShouldConnectWhenProvidedWithValidUrl() throws Exception {
    }
  }

+  @Test
+  public void testParameters() throws Exception {


I'd be happy to receive pointers on how to properly unit test this, since we will probably need to remove this integration test before submitting as a non-draft PR.

alamb · 2023-01-31T21:52:18Z

I also happen to be working on support for parameterized statements in InfluxDB IOx (WIP at https://github.com/influxdata/influxdb_iox/pull/6790) and I had just hit this. I plan to try and test / review this PR tomorrow

lidavidm · 2023-01-31T21:54:28Z

note there was a prior PR in the issue description, I don't know if that answers any of your questions here

avantgardnerio · 2023-02-01T04:08:25Z

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

+      FieldVector v = new
+    }
+    VectorSchemaRoot parameters = new VectorSchemaRoot();
+    this.preparedStatement.setParameters();


I think this is the missing piece. If we can translate the AvaticaParameters into a VectorSchemaRoot the server will get what it needs.

github-actions · 2023-02-01T14:31:51Z

Closes: [Java][FlightRPC] FlightSQL error: 'Parameter ordinal out of range' executing a prepared stmt with params #33475

alamb · 2023-02-01T15:25:36Z

Previous PR #14627

avantgardnerio · 2023-02-01T16:25:25Z

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

+      }
+    }
+    VectorSchemaRoot parameters = new VectorSchemaRoot(fields);
+    this.preparedStatement.setParameters(parameters);


Step #2 of this PR seems complete: now we can both:

Get the schema from the server

Set the parameter values to pass to the server

avantgardnerio · 2023-02-01T20:17:22Z

Latest update: everything is working (if not a little ugly)

Unfortunately it's working against a proprietary server, so I'll begin the work to put it into the example server in arrow-rs.

Then I could use some help getting this PR ready for merge.

lidavidm · 2023-02-01T20:26:12Z

The prior PR uses a mocked server for unit testing - we can do that here, right?

avantgardnerio · 2023-02-02T16:04:44Z

The prior PR uses a mocked server for unit testing - we can do that here, right?

Yes, I'll look into it. Happy to pull over those changes from your PR if that's the easiest way.

lidavidm · 2023-03-03T20:16:00Z

@avantgardnerio was there a problem with the PR?

avantgardnerio · 2023-03-06T17:49:52Z

@avantgardnerio was there a problem with the PR?

Oh, you're right - I hadn't looked at this branch in so long I thought it was stale, but I do think this needs to go in.

amol- · 2023-03-30T17:26:00Z

Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍

sukwanlee · 2023-09-04T22:36:16Z

Will prepared statements be supported anytime in the near future?

lidavidm · 2023-09-05T01:20:39Z

Contributions are welcome. I'm not aware of any active development on the JDBC driver.

aiguofer · 2023-09-05T01:49:06Z

This will be a priority for our team within the next few months. If no one has done it by then I will likely take this up.

avantgardnerio · 2023-09-05T14:52:07Z

@aiguofer what I have in this branch basically works, I think it just needs to cleanup and tests to merge. Sorry I don't have time to help right now...

aiguofer · 2023-09-05T17:29:35Z

Sweet that's what I'm hoping for. I'd likely strip out all the metrics stuff since it's unrelated, but I'm hoping between this PR and @lidavidm's original PR that it should be pretty easy to put together. We have some other higher priorities right now but in a month or so I might be able to get to this (or if I somehow find myself extremely bored on a weekend before then haha).

…C driver (#38404) 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. * Closes: #33475 * Closes: #35536 Authored-by: Diego Fernandez <aiguo.fernandez@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

…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 <aiguo.fernandez@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

avantgardnerio requested a review from lidavidm January 31, 2023 21:02

github-actions bot added the Component: Java label Jan 31, 2023

avantgardnerio mentioned this pull request Jan 31, 2023

[Java][FlightRPC] FlightSQL error: 'Parameter ordinal out of range' executing a prepared stmt with params #33475

Closed

avantgardnerio changed the title ~~Send prepared statement parameters~~ [GH-33475]: [flight-sql] Send prepared statement parameters Jan 31, 2023

avantgardnerio requested a review from alamb January 31, 2023 21:05

avantgardnerio commented Jan 31, 2023

View reviewed changes

lidavidm reviewed Jan 31, 2023

View reviewed changes

avantgardnerio commented Jan 31, 2023

View reviewed changes

avantgardnerio commented Feb 1, 2023

View reviewed changes

lidavidm changed the title ~~[GH-33475]: [flight-sql] Send prepared statement parameters~~ GH-33475: [Java][FlightRPC] Send prepared statement parameters Feb 1, 2023

avantgardnerio commented Feb 1, 2023

View reviewed changes

avantgardnerio requested a review from lidavidm February 1, 2023 20:17

Squash

6277f58

avantgardnerio force-pushed the bg_params branch from bdf71ae to 6277f58 Compare February 3, 2023 16:24

Brent Gardner added 2 commits February 3, 2023 10:55

setSafe seems to work

a0619a2

Failing test

6bb6878

Brent Gardner added 3 commits February 7, 2023 11:04

Failing subquery test

36e3b7f

q6 perf test

c7e2333

Takes just as long to select the same hard-coded values

a7f4d11

avantgardnerio closed this Mar 2, 2023

Brent Gardner added 6 commits March 2, 2023 10:20

Add instrumentation to determine which calls are slow

e1e9b92

More metrics

8fca1f4

Remove useless counters

a8502aa

Down the rabbit hole

b799324

Can authenticate FlightClient

85b767c

Can get results calling sqlClient directly

827da92

avantgardnerio reopened this Mar 6, 2023

Brent Gardner added 5 commits March 7, 2023 10:27

Perf testing

a356331

Failing double digit parameter test

6b62b89

Allocate enough space for bulk insert queries

1880967

Remove debugging

ccf12c9

Remove debugging

8076902

amol- closed this Mar 30, 2023

alamb mentioned this pull request Sep 14, 2023

[FlightSQL] Stateless prepared statement with parameter support #37720

Closed

aiguofer mentioned this pull request Oct 23, 2023

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

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-33475: [Java][FlightRPC] Send prepared statement parameters #33961

GH-33475: [Java][FlightRPC] Send prepared statement parameters #33961

avantgardnerio commented Jan 31, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Jan 31, 2023

lidavidm commented Jan 31, 2023

avantgardnerio Jan 31, 2023

avantgardnerio Jan 31, 2023

lidavidm Jan 31, 2023

avantgardnerio Jan 31, 2023

avantgardnerio Jan 31, 2023

avantgardnerio Jan 31, 2023

lidavidm commented Jan 31, 2023

avantgardnerio Jan 31, 2023

avantgardnerio Jan 31, 2023

alamb commented Jan 31, 2023

lidavidm commented Jan 31, 2023

avantgardnerio Feb 1, 2023

github-actions bot commented Feb 1, 2023

alamb commented Feb 1, 2023

avantgardnerio Feb 1, 2023

avantgardnerio commented Feb 1, 2023

lidavidm commented Feb 1, 2023

avantgardnerio commented Feb 2, 2023

lidavidm commented Mar 3, 2023

avantgardnerio commented Mar 6, 2023

amol- commented Mar 30, 2023

sukwanlee commented Sep 4, 2023

lidavidm commented Sep 5, 2023

aiguofer commented Sep 5, 2023

avantgardnerio commented Sep 5, 2023

aiguofer commented Sep 5, 2023

GH-33475: [Java][FlightRPC] Send prepared statement parameters #33961

GH-33475: [Java][FlightRPC] Send prepared statement parameters #33961

Conversation

avantgardnerio commented Jan 31, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jan 31, 2023

lidavidm commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 31, 2023

lidavidm commented Jan 31, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 1, 2023

alamb commented Feb 1, 2023

Choose a reason for hiding this comment

avantgardnerio commented Feb 1, 2023

lidavidm commented Feb 1, 2023

avantgardnerio commented Feb 2, 2023

lidavidm commented Mar 3, 2023

avantgardnerio commented Mar 6, 2023

amol- commented Mar 30, 2023

sukwanlee commented Sep 4, 2023

lidavidm commented Sep 5, 2023

aiguofer commented Sep 5, 2023

avantgardnerio commented Sep 5, 2023

aiguofer commented Sep 5, 2023

avantgardnerio commented Jan 31, 2023 •

edited by github-actions bot

Loading