-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-34742: [Java] Split flight-sql-jdbc-driver to facilitate reuse #34678
Conversation
|
flight-sql-jdbc-driver module contains a set of utilities to implement JDBC support backed by arrow vectors. However, it employs maven shade plugin which is hard to use. This patch splits it into two separate modules.
@lidavidm Could you please take a look? Thanks! |
@davisusanibar or @lwhite1 would either of you be able to take a first pass? Also, Lines 799 to 803 in ce0d20c
|
Fixed BTW, though this PR looks very large by affecting hundreds of files, it contains only 200- lines of change as mostly are file relocation. |
@github-actions crossbow submit java-jars |
Revision: d382b46 Submitted crossbow builds: ursacomputing/crossbow @ actions-5a76b30651
|
@assignUser Should we rerun the crossbow build? |
Revision: 7067c95 Submitted crossbow builds: ursacomputing/crossbow @ actions-c5e181f11b
|
Failure is in |
I re-ran the job and the job passed. :-) |
cc @sunchao :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on CI changes, can't comment on the rest :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. It seems currently flight-sql-jdbc-driver
just shade everything so it's difficult for downstream projects to manage dependencies (e.g., exclude certain dependency from flight-sql-jdbc-driver
).
I'm not a regular reviewer for Arrow-Java though so it's better for @lidavidm to make the final call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. However, our testing of the driver artifact itself is nonexistent. We should consider setting up a job that takes the built driver JAR and runs it against the dremio-oss Docker container or similar.
|
||
<artifactId>flight-sql-jdbc-core</artifactId> | ||
<name>Arrow Flight SQL JDBC Driver Core</name> | ||
<description>(Contrib/Experimental) A JDBC driver based on Arrow Flight SQL.</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we update this to reflect that this isn't the actual driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly modified the description. Please correct me if there is any better description.
Do you have any idea why the https://github.com/apache/arrow/actions/runs/4527686135/jobs/7973768071?pr=34678 |
The test probably needs some debugging, but I have very little time for Java work these days (it's mostly on my personal time). There haven't been substantial contributions to the driver since its initial contribution. |
I tried to debug it but cannot reproduce on my Mac. It seems to be unstable and now the tests are all green. |
Thanks for looking. I filed apache/arrow-java#210 for anyone who wants to follow up. |
I'll open a new issue for this, because the original issue is quite different. |
|
Benchmark runs are scheduled for baseline = 0a21a4c and contender = eaa1a1e. eaa1a1e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
…se (apache#34678) ### Rationale for this change `flight-sql-jdbc-driver` module contains a set of utilities to implement JDBC support backed by arrow vectors. However, it employs maven shade plugin which is hard to make it a dependecy of external projects. ### What changes are included in this PR? This patch splits `flight-sql-jdbc-driver` module into two separate modules: `flight-sql-jdbc-core` and `flight-sql-jdbc-driver`. `flight-sql-jdbc-core` contains almost every thing in the original module except the shade plugin. `flight-sql-jdbc-driver` only shades `flight-sql-jdbc-core`. ### Are these changes tested? Make sure all tests pass. ### Are there any user-facing changes? No. * Closes: #20730 * Closes: apache#34742 Authored-by: Gang Wu <ustcwg@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
|
Rationale for this change
flight-sql-jdbc-driver
module contains a set of utilities to implement JDBC support backed by arrow vectors. However, it employs maven shade plugin which is hard to make it a dependecy of external projects.What changes are included in this PR?
This patch splits
flight-sql-jdbc-driver
module into two separate modules:flight-sql-jdbc-core
andflight-sql-jdbc-driver
.flight-sql-jdbc-core
contains almost every thing in the original module except the shade plugin.flight-sql-jdbc-driver
only shadesflight-sql-jdbc-core
.Are these changes tested?
Make sure all tests pass.
Are there any user-facing changes?
No.