Skip to content

Conversation

@jduo
Copy link
Member

@jduo jduo commented Apr 7, 2022

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

iurysalino and others added 30 commits March 29, 2022 10:09
* Address all ratification comments with small fixes

* Small fixes

* Revert small changes

* Add more exception messages

* Remove ExceptionTemplateThrower

* Change UnsupportedOperationException to SQLException

* Address all ratification comments with small fixes

* Fix failing tests

* Address to James' review

* Removed some suppressions by creating newChildAllocators

* Fix CONNECTION_STRING_EXPECTED

* nit on CONNECTION_STRING_EXPECTED

* Address more review comments

* nit on holder comment

* Remove FindBugs annotations

* Fix rebase issues

* Downgraded JUnit version back to what is defined in java/pom.xml

* Remove JUnit 5 usage

* Fix import order

* Format rebased commit

* nit: add final
[JAVA] [JDBC] Jdbc tls connection with disable certification verification
This PR adds the feature to include the filename of a fragment if it is a FileFragment as a scalar value and includes the augmented fields in the dataset schema

Closes apache#12560 from sanjibansg/filename_retrieval

Authored-by: Sanjiban Sengupta <[email protected]>
Signed-off-by: Weston Pace <[email protected]>
https://nvd.nist.gov/vuln/detail/CVE-2018-25032
zlib before 1.2.12 may cause memory corruption

Closes apache#12760 from cyb70289/16078-zlib

Authored-by: Yibo Cai <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
…tObject-getString' into change-interval-behaviour-for-getObject-getString
jvictorhuguenin and others added 9 commits April 21, 2022 11:47
Add support for ";" as parameter divider
Implement toString method for JdbcArray class
* 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
@lidavidm
Copy link
Member

lidavidm commented May 2, 2022

@jduo do you still plan to rebase things? Also, would you like the base branch to be updated since the proposal was merged?

@ZMZ91
Copy link
Contributor

ZMZ91 commented May 26, 2022

Hi there,
Doe this support auth token?

@ZMZ91
Copy link
Contributor

ZMZ91 commented May 31, 2022

Hi, I've tried this code and found it supports auth token only when set the server auth handler as NoOpAuthHandler and add an auth header in middleware to pass the token at the same time, right?

@lidavidm
Copy link
Member

Looks like everyone's filed ICLAs. I'll get the paperwork filed ASAP

public List<FlightStream> getStreams(final FlightInfo flightInfo) {
return flightInfo.getEndpoints().stream()
.map(FlightEndpoint::getTicket)
.map(ticket -> sqlClient.getStream(ticket, getOptions()))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this behavior is incorrect, and it is causing problems for DataFusion's implementation. It looks like this code re-uses the existing sqlClient, ignoring the value of flightInfo.enpoints[x].locations[y].uri (which may be different than the original URI the sqlClient connected to, in the case of DataFusion, the sqlClient would be connected to the scheduler at one URI, and the data would be held on one or more executor nodes at different URIs).

I can attempt a PR if you agree with my hypothesis. I'm new to all this, so I could be way off base. And thank you for putting in the massive amount of work necessary to make this BTW!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, we actually merged a clarification for this behavior a little while ago. #12636

Note we're still finalizing the IP clearance of this PR (but I assume it will go through one way or another)

Copy link

@avantgardnerio avantgardnerio Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to hear it's been fixed. Is there a current, long-running repo & branch we can track until this gets merged into arrow proper?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the repo/branch being used for the IP clearance so I would say you're already looking at it!

My plan is once the clearance goes through, this will get merged into a branch on apache/arrow (the PR merge target here, flight-sql-jdbc), then we'll merge master into the branch, then finally do a PR into master (and review the code and such).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think I understand now:

merged a clarification for this behavior

The behavior has been clarified, but the code to which I linked has not been corrected according to the now clarified expected behavior?

If that's the case, I'll work on a fix and PR this branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you got it. We clarified the spec, but haven't necessarily updated everything else

Copy link

@avantgardnerio avantgardnerio Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but I'm interested in feedback about how to do it the correct way: rafael-telles#42

I'm also happy to help others get set up to run with a similar test environment as me if that helps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see James followed up already. Thanks for taking a look!

@lidavidm lidavidm changed the title ARROW-15452: [FlightRPC][Java] JDBC driver for Flight SQL ARROW-15452: [FlightRPC][Java] JDBC driver for Flight SQL Jul 29, 2022
@lidavidm lidavidm merged commit e7a7251 into apache:flight-sql-jdbc Jul 29, 2022
@github-actions
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.