search

apache / arrow-adbc

Database connectivity API standard and libraries for Apache Arrow
https://arrow.apache.org/adbc/
Apache License 2.0
385 stars 98 forks source link

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

Open laurentgo opened 1 month ago

laurentgo commented 1 month ago

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/arrow-adbc#2237

lidavidm commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

I had a couple of questions about some of the interfaces

What questions?

paleolimbot commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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