OK, I’ll change my vote to -1. I think you should pick up my code and try to complete upgrading Calcite. Then you’d get a feel for the issues I was seeing.
I do think that we should convert int[] to long[]; other parts of Avatica went from int to long for update-counts quite a while ago, and to not do it for this one introduces a painful inconsistency. But the others I’ll leave up to you. I think it’s OK if close() no-ops if the statement doesn’t exist, because I can’t think of an application that would actually want to handle that precise condition. No urgency. We have time to get this right. Julian > On May 23, 2016, at 7:16 PM, Josh Elser <[email protected]> wrote: > > tl;dr Lots of great finds, most of which I think should go into a 1.8.0 > (rather than have to work around them later). Limited availability will > probably keep me from fixing them until Thursday, but I am happy to work on > them then. > > What's your take on switching your vote on avatica-1.8.0-rc0 based on the > below? I certainly won't have my feelings hurt, I'm rather happy you found > all of this before we made a release :) > > Julian Hyde wrote: >> And also: >> * Clarify what "closeStatement(StatementHandle)” should do if the statement >> handle is invalid. Throw or do nothing? > > It looks like JdbcMeta logs a debug message and does not throw an error if > the provided handle does not exist. We could clarify this by returning back > some enum value: > > enum Result { > CLOSED, // Successfully closed > MISSING, // Did not refer to a known Statement > FAILED_CLOSE // Failed (SQLException) closing the Statement > } > > We could have a nice rich response instead of the (essentially) empty > response. I am not swayed one way or the other that we'd need to do this for > 1.8.0. > >> I tried to upgrade Calcite to Avatica 1.8.0 and it’s not trivial. I have >> pushed my work to the branch >> https://github.com/julianhyde/calcite/tree/1280-upgrade-avatica<https://github.com/julianhyde/calcite/tree/1280-upgrade-avatica>. >> I’d like to make sure that implementing the new APIs is possible before we >> approve the Avatica 1.8.0 release. Can someone take a look? >> > > I'll try to take a look tonight. Admittedly, I'm a bit jet-lagged already and > will be at {HBase,Phoenix}Con Tuesday and Wednesday so my availability might > be more sparse than normal. > >> >>> On May 23, 2016, at 1:40 AM, Julian Hyde<[email protected]> wrote: >>> >>> A couple of other problems/questions: >>> * There still calls within Avatica to the deprecated form of >>> prepareAndExecute. Wouldn’t it be wise to upgrade all internal uses? > > It would certainly be proactive to do so. The code I added to make sure that > older clients still work is (unintentionally) tested by this, but it's > admittedly silly that I didn't switch over all of the uses. > > Not a blocker for 1.8.0, but would be nice to fix if we're doing another RC > (see below) > >>> * Why does prepareAndExecuteBatch not have a PrepareCallback parameter like >>> prepareAndExecute? It makes it difficult to implement it in the obvious way. > > Omission on my part. We should fix this for 1.8.0 IMO. > >>> >>> >>>> On May 23, 2016, at 1:13 AM, Julian >>>> Hyde<[email protected]<mailto:[email protected]>> wrote: >>>> >>>> I just logged Avatica issue >>>> https://issues.apache.org/jira/browse/CALCITE-1254<https://issues.apache.org/jira/browse/CALCITE-1254>, >>>> "Support PreparedStatement.executeLargeBatch”. >>>> >>>> Would fixing cause an incompatible change to the protocol? If so, we >>>> should consider putting it into 1.8. > > We could fix it in a backwards compatible manner now, but it would be trivial > to just change now before the original release (and my preference). It's > certainly less churn for downstream people to find out they have to look at > another field for the "proper" value. > > I didn't even think about the new methods in JDK8. I think we should get this > fixed for 1.8.0.
