Skip to content
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

feat(java): Standardize core exceptions to AdbcException #2247

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

laurentgo
Copy link
Contributor

ADBC base classes/interfaces are not consistent in using AdbcException for all their methods (sometimes Exception or IOException are used).

Introduces AdbcCloseable which is a subinterface of AutoCloseable throwing AdbcException instead of Exception and replace usage of AutoCloseable in core interfaces with AdbcCloseable

Also replaces use of IOException in QueryResult with AdbcException.

Fixes #2237

ADBC base classes/interfaces are not consistent in using `AdbcException`
for all their methods (sometimes `Exception` or `IOException` are used).

Introduces `AdbcCloseable` which is a subinterface of `AutoCloseable`
throwing `AdbcException` instead of `Exception` and replace usage of
`AutoCloseable` in core interfaces with `AdbcCloseable`

Also replaces use of `IOException` in `QueryResult` with
`AdbcException`.

Fixes apache#2237
@github-actions github-actions bot added this to the ADBC Libraries 15 milestone Oct 14, 2024
@lidavidm
Copy link
Member

We wanted to treat core/ as part of the 'spec' and try not to make changes to it without a vote. Additionally we did want to try to preserve backwards compatibility...While this just changes the exception specifier for some methods, that's still a breaking change as you note.

@zeroshade @paleolimbot any opinions here?

@laurentgo
Copy link
Contributor Author

Sorry, didn't know the rules but I don't mind voting on it and/or start a discussion on arrow-devel. I guess while I was getting familiar with the code I had a couple of questions about some of the interfaces

@lidavidm
Copy link
Member

I had a couple of questions about some of the interfaces

What questions?

@paleolimbot
Copy link
Member

I am not familiar with the Java conventions around this kind of thing, although it seems like this is not any change in semantics around calling any particular method, just an improvement in error handling. This is a breaking change because somebody who implemented a driver in Java would have to update their throws specification?

@lidavidm
Copy link
Member

I am not familiar with the Java conventions around this kind of thing, although it seems like this is not any change in semantics around calling any particular method, just an improvement in error handling. This is a breaking change because somebody who implemented a driver in Java would have to update their throws specification?

Yes, and potentially users would have to update their catches (since you have to explicitly catch what is declared by throws)

@paleolimbot
Copy link
Member

Got it. It does seem like a good change to do and a good one to do sooner than later (e.g., before we wrap all the C drivers and make them accessible in Java). It does seem "spec level", although I am not sure that bumping a major version would communicate that change very well. If it's possible to make this change before we make all the C drivers available in Java I think it would be a good thing?

@lidavidm
Copy link
Member

Ok. I'd be OK with bending the 'rules' here a bit just so long as something makes it to the mailing list to notify people

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Java] Normalize exceptions to AdbcException subclasses
3 participants