You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Julian Hyde <jh...@apache.org> on 2016/05/23 08:13:42 UTC
Should Avatica's executeBatch return int[] or long[]?
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.
Julian
Re: Should Avatica's executeBatch return int[] or long[]?
Posted by Julian Hyde <jh...@apache.org>.
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 <jo...@gmail.com> 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<jh...@apache.org> 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<jh...@apache.org>> 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.
Re: Should Avatica's executeBatch return int[] or long[]?
Posted by Josh Elser <jo...@gmail.com>.
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)\u201d 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\u2019s 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\u2019d 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<jh...@apache.org> wrote:
>>
>> A couple of other problems/questions:
>> * There still calls within Avatica to the deprecated form of prepareAndExecute. Wouldn\u2019t 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<jh...@apache.org>> 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\u201d.
>>>
>>> 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.
Re: Should Avatica's executeBatch return int[] or long[]?
Posted by Julian Hyde <jh...@apache.org>.
And also:
* Clarify what "closeStatement(StatementHandle)” should do if the statement handle is invalid. Throw or do nothing?
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?
Julian
> On May 23, 2016, at 1:40 AM, Julian Hyde <jh...@apache.org> 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?
> * Why does prepareAndExecuteBatch not have a PrepareCallback parameter like prepareAndExecute? It makes it difficult to implement it in the obvious way.
>
> Julian
>
>
>> On May 23, 2016, at 1:13 AM, Julian Hyde <jhyde@apache.org <ma...@apache.org>> 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.
>>
>> Julian
>>
>
Re: Should Avatica's executeBatch return int[] or long[]?
Posted by Julian Hyde <jh...@apache.org>.
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?
* Why does prepareAndExecuteBatch not have a PrepareCallback parameter like prepareAndExecute? It makes it difficult to implement it in the obvious way.
Julian
> On May 23, 2016, at 1:13 AM, Julian Hyde <jh...@apache.org> 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.
>
> Julian
>