ARROW-15452: [FlightRPC][Java] JDBC driver for Flight SQL #12254

rafael-telles · 2022-01-25T14:15:06Z

This implements a JDBC driver able to communicate to Flight SQL sources.

So far this covers:

Metadata retrieval by DatabaseMetadata, ResultSetMetadata, etc.
Query execution by statements and prepared statements

Yet to be done:

Parameter binding on prepared statements

github-actions · 2022-01-25T14:41:17Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

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

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

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

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

See also:

github-actions · 2022-01-25T16:48:49Z

https://issues.apache.org/jira/browse/ARROW-15452

github-actions · 2022-01-25T16:48:50Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

lidavidm

Thanks. I've done an initial pass but haven't looked at things too deeply.

Does having benchmarks make sense? (Maybe for particular components like accessor/transformer?)

lidavidm · 2022-01-26T14:30:45Z

java/pom.xml

+          <artifactId>spotbugs-maven-plugin</artifactId>
+          <version>4.2.3</version>
+        </plugin>
+


Is it possible to keep these plugins/dependencies in the driver's pom.xml? (Though it would be nice to enable static analyzers project-wide, that should be done separately.)

lidavidm · 2022-01-26T14:31:35Z

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

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


I'm going to assume any changes under flight-sql are from the other PRs…let me know if that's not the case.

It's from the ColumnMetadata PR, we needed it to continue implementing some missing stuff like proper ResultSetMetaData for PreparedStatement metadata, and other metadata for ResultSet#getColumns.

lidavidm · 2022-01-26T14:33:41Z

java/flight/flight-jdbc-driver/src/test/resources/keys/cert0.pem

@@ -0,0 +1,29 @@
+-----BEGIN CERTIFICATE-----


We have certificates in /testing/data/flight, could those be used? (I guess we need to make a keystore for them.)

We could use them, we just need to get it in flight-jdbc-driver, and see how we can make a keystore for them.

lidavidm · 2022-01-26T14:35:40Z

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

+      return new ArrowFlightJdbcNullVectorAccessor(setCursorWasNull);
+    }
+
+    throw new UnsupportedOperationException();


Maybe include the type in the exception message?

This doesn't seem to have changed

lidavidm · 2022-01-26T14:35:49Z

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

+    } else if (vector instanceof DenseUnionVector) {
+      return new ArrowFlightJdbcDenseUnionVectorAccessor((DenseUnionVector) vector, getCurrentRow,
+          setCursorWasNull);
+    } else if (vector instanceof NullVector || vector == null) {


Can a null vector ever occur?

It could occur if the Flight SQL Server Endpoint defines a column with it. It could represent java.sql.Types.NULL.

I mean specifically null, not NullVector, sorry

lidavidm · 2022-01-26T18:02:55Z

...t/flight-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/utils/ConnectionWrapper.java

+/**
+ * Auxiliary wrapper class for {@link Connection}, used on {@link ArrowFlightJdbcPooledConnection}.
+ */
+public class ConnectionWrapper implements Connection {


Since this appears to be a pure proxy, it might be worth noting that fact (aka this is purely to serve as a base class and factor out the boilerplate elsewhere)

Do you want us to document that this is to remove boilerplate?

lidavidm · 2022-01-26T18:04:22Z

...t-jdbc-driver/src/main/java/org/apache/arrow/driver/jdbc/utils/ExceptionTemplateThrower.java

+   *
+   * @return the exception.
+   */
+  public static UnsupportedOperationException getOperationNotSupported(final Class<?> type) {


It seems this is used purely in the base Accessor class, maybe it could be a private static method of that class?

lidavidm · 2022-01-26T18:07:54Z

java/flight/flight-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/utils/UrlSample.java

+ * @deprecated not updatable to match dinamic server allocation.
+ */
+@Deprecated
+public enum UrlSample {


Why are we adding a new deprecated class?

We're cleaning up deprecated classes/methods.

lidavidm · 2022-01-26T18:09:14Z

...ht/flight-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/utils/PropertiesSample.java

+ * @see FlightServerTestRule
+ * @deprecated not updatable to match dinamic server allocation.
+ */
+@Deprecated


Ditto here, why add a class only to immediately deprecate it? (I guess things are in the middle of a refactoring here?)

Yes! This is still work in progress and we will have to clean up

lidavidm · 2022-01-26T18:11:03Z

...ight-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/utils/FlightStreamQueueTest.java

+    try {
+      queue.next();
+    } catch (final IllegalStateException e) {
+      expectedExceptionOnNextIfClosed = Optional.of(e);


It seems other parts of the codebase use assertThrows for this type of check

rafael-telles · 2022-01-26T22:36:55Z

Thank you so much @lidavidm ! We will be working on your feedback soon

wesm · 2022-02-04T23:13:54Z

I just wanted to echo my comment from the mailing list that I think it would be best if we held a code donation vote and IP clearance for this piece of work since it is substantial and was developed outside of the Arrow community. I don't expect there will be any issues but it is good due diligence. As a reminder we welcome the development of large new projects in feature branches, where the development process otherwise is conducted according to the Apache Way (public discussions, issues, PRs, etc.)

I believe this will involve a Software Grant from Dremio (correct me if I'm wrong) for the work and ICLAs for the individual contributors.

lidavidm · 2022-02-04T23:18:04Z

Thanks for chiming in Wes - I'll look into the process and follow up on the mailing list @rafael-telles.

lidavidm · 2022-03-10T21:37:19Z

BTW, please let us know when you think this is ready for a full review

jduo · 2022-03-11T18:28:25Z

...bc-driver/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java

+   * @return a {@code FlightStream} of results.
+   */
+  public List<FlightStream> getStreams(final FlightInfo flightInfo) {
+    return flightInfo.getEndpoints().stream()


This isn't correct. This will try to get every ticket using the current client. However each endpoint is a pair <Ticket, List>.

What we actually need to do is for each endpoint, create a FlightClient at each location and call getStream() with the associated ticket. See:

arrow/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/IntegrationTestClient.java

Line 151 in dc2e0b2

for (FlightEndpoint endpoint : info.getEndpoints()) {

Note that if the list of locations is empty, it means the data is only available at the current location:

Can optimize this to some extent if the current location is used once, use the current client, though that limits the driver to only running one Statement at a time.

We should consider pooling FlightClients. I think Apache Pools is a good choice here, and use KeyedObjectPool to model the problem: https://commons.apache.org/proper/commons-pool/apidocs/org/apache/commons/pool2/KeyedObjectPool.html

lidavidm · 2022-03-25T21:26:31Z

This can be rebased against the accepted proposals now.

…tor the code to use it

…d refactor tests to use it

… Timestamp types

… accessor

…ods to use this interface

Implement toString method for JdbcArray class

Fix struct type on jdbc

* Add toString to Time obj in Time#toString * Improve Time toString * Fix maven plugins * Revert "Update java/flight/flight-jdbc-driver/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcTimeStampVectorAccessorTest.java" This reverts commit 00808c0. * Revert "Merge pull request #29 from rafael-telles/Timestamp_fix" This reverts commit 7924e7b, reversing changes made to f6ac593. * Fix DateTime for negative epoch * Remove unwanted change * Fix negative timestamp shift * Fix coverage * Refator DateTimeUtilsTest

…ervals

…ctly

…ormat Change the behavior to return the correct format for day and year intervals.

Fix query exception message

…etadata-type-name [Java] [JDBC] Flight jdbc driver create metadata type name.

pitrou · 2022-06-07T11:51:03Z

@rafael-telles It seems this needs rebasing on git master and then fixing conflicts, would you like to do that?

Fix attributes as headers bug and token value parsing error

Implement case insensitive for the property keys

lidavidm · 2022-08-03T19:46:00Z

We should probably close this PR in favor of a new PR based on the cleared code

amol- · 2023-03-30T17:18:27Z

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

github-actions bot added the Component: Java label Jan 25, 2022

rafael-telles changed the title ~~[FlightRPC][Java] JDBC driver for Flight SQL~~ ARROW-15452: [FlightRPC][Java] JDBC driver for Flight SQL Jan 25, 2022

lidavidm self-requested a review January 25, 2022 21:05

lidavidm reviewed Jan 26, 2022

View reviewed changes

jduo reviewed Mar 11, 2022

View reviewed changes

vfraga mentioned this pull request Mar 14, 2022

Address all JDBC ratification comments with small fixes #1 rafael-telles/arrow#14

Merged

jcralmeida and others added 16 commits March 29, 2022 10:09

Add tests to check authentication with token

3f28b1e

Add a method populateProperties on Authentication interface and refac…

1ef15f5

…tor the code to use it

Add a method to create a FlightServerTestRule with standard values an…

ede0e63

…d refactor tests to use it

add try with resources on TokenAuthenticationTest

bf8722e

Add ClientCookieMiddleware to ArrowFlightSqlClientHandler

f440890

Add test for the new functionality of handling cookies

3e5e986

Refactor cookie code

3bf0aab

Validate the cookies before insert it on server

529a07d

Include Statement in the try with resources.

9053892

Include Statement in the try with resources.

8b2717a

Clean up Cookies test

51bb218

Change Illegal Argument Exception to SQL Exception for Date, Time and…

86b3021

… Timestamp types

Change Illegal Argument Exception to SQL Exception for varchar vector…

3c1b1bb

… accessor

Instantiate a checked function interface and change the calls on meth…

ed37fc6

…ods to use this interface

Remove unnecessary try catch

b60bec3

Apply checkstyle

1a4df19

jcralmeida and others added 14 commits April 22, 2022 16:32

Merge pull request #35 from rafael-telles/to-string-array-jdbc-review

e2fee64

Implement toString method for JdbcArray class

Merge pull request #37 from rafael-telles/fix-struct-type-on-jdbc

4883821

Fix struct type on jdbc

Change VarCharAccessor Getter to Bytes rather than Text (#38)

e745372

Change getMillisReprValue to greater or equals

5f07a0e

Fix query exception message

792a29c

Change the behavior to return the correct format for day and year int…

55fc9a1

…ervals

Test if holder.isSet indicates the value is null and return null dire…

133d809

…ctly

Change the getString and getObject methods

6347675

Include a test passing a Joda object

bd16af2

Remove checkstyle problem

7f09eb0

Merge pull request #40 from rafael-telles/fix-day-and-year-interval-f…

7f1eeec

…ormat Change the behavior to return the correct format for day and year intervals.

Merge pull request #39 from rafael-telles/fix-exception-messages

e383cef

Fix query exception message

Merge pull request #30 from rafael-telles/flight-jdbc-driver-create-m…

c9b722d

…etadata-type-name [Java] [JDBC] Flight jdbc driver create metadata type name.

github-actions bot added Component: FlightRPC Component: C++ labels May 10, 2022

Temporarily disable Spotbugs

c9cdcac

jvictorhuguenin and others added 7 commits July 27, 2022 13:48

Fix attributes as headers bug and token value parsing error

360ee52

Fix crash for empty properties on the url

d66a60a

Merge pull request #43 from rafael-telles/fix-all-attributes-as-headers

1da5ba8

Fix attributes as headers bug and token value parsing error

Implement case insensitive for the property keys

e22f7e4

Fix property lowercasing only for the connection string

b2af3a0

Add unit tests and fix possible crash on the parser

6eb3cfa

Merge pull request #44 from rafael-telles/implement-case-insensitive

220e844

Implement case insensitive for the property keys

Fix checkstyle issues

6c8a900

asfimport mentioned this pull request Sep 30, 2022

[FlightRPC][Java] JDBC driver for Flight SQL #30931

Closed

amol- closed this Mar 30, 2023

ARROW-15452: [FlightRPC][Java] JDBC driver for Flight SQL #12254

ARROW-15452: [FlightRPC][Java] JDBC driver for Flight SQL #12254

Uh oh!

Conversation

rafael-telles commented Jan 25, 2022

Uh oh!

github-actions bot commented Jan 25, 2022

Uh oh!

github-actions bot commented Jan 25, 2022

Uh oh!

github-actions bot commented Jan 25, 2022

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfraga Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafael-telles commented Jan 26, 2022

Uh oh!

wesm commented Feb 4, 2022

Uh oh!

lidavidm commented Feb 4, 2022

Uh oh!

lidavidm commented Mar 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Mar 25, 2022

Uh oh!

pitrou commented Jun 7, 2022

Uh oh!

lidavidm commented Aug 3, 2022

Uh oh!

amol- commented Mar 30, 2023

Uh oh!

Reviewers

vfraga Mar 10, 2022 •

edited

Loading