-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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-41947: [Java] Support catalog in JDBC driver with session options #42035
Conversation
|
Overall LGTM! |
...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java
Outdated
Show resolved
Hide resolved
...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java
Outdated
Show resolved
Hide resolved
...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java
Outdated
Show resolved
Hide resolved
356d4bd
to
cfad7c2
Compare
All updates have been incorporated. |
@stevelorddremio thanks, do you mind rebasing/running the new formatter? |
...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java
Outdated
Show resolved
Hide resolved
...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java
Outdated
Show resolved
Hide resolved
cfad7c2
to
e6a0b63
Compare
…tions Spotless update
I think AMD64 Windows Server 2022 Java JDK 11 will pass if re-triggered. |
...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java
Show resolved
Hide resolved
…tions PR comments
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.
Sorry for the back and forth. This looks good to me now!
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit cecd771. There was 1 benchmark result indicating a performance regression:
The full Conbench report has more details. It also includes information about 16 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Hi, I was testing this change and I am facing a problem when the I am receiving the following error invoking the method.
I think the reason is the following. Checking this code: Lines 174 to 189 in 6a2e19a
Lines 174 to 189 in 6a2e19a
I think that line line 181 Also, I would remove the is if: Lines 222 to 225 in 6a2e19a
why not invoking the closeSession when the catalog was not set?
|
Please, let me know if I have to open a new issue with this problem. I put here as first step because this version 17 is being currently published. |
Please open a new issue |
@github-actions crossbow submit -g java |
Revision: 9a43834 Submitted crossbow builds: ursacomputing/crossbow @ actions-f7d1d505f9 |
Rationale for this change
See Issue #41947
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
Introductiona of an optional catalog query parameter in the JDBC url string.