Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Am 19.01.2013 16:08, schrieb Lance Andersen - Oracle: I looked at the latest webrev and it seems okay to me. SQLInputImpl.getNextAttribute would be bit better if it only incremented idx after retrieving an attribute but that is existing code. Minor intent issue in SQLOutputImpl. writeNClob. fixed the spacing. going to leave idx alone for now as but will look at this for the future ... and starting idx by 0 instead -1 would not be so exotic, see: internal review ID of 2426775 -Ulf
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
On Jan 19, 2013, at 9:43 AM, Alan Bateman wrote: > On 15/01/2013 16:48, Lance Andersen - Oracle wrote: >> Here is a revision >> >> http://cr.openjdk.java.net/~lancea/8006139/webrev.01 >> >> I still have to enter the JBS entry for the javadoc clarifications (and I >> also found another javadoc issue that was due to incorrect cut& paste when >> the code was written) and ccc request >> >> As i mentioned in an earlier thread, these classes are hardly ever, if at >> all used and would only be used when UDTs are used and the majority of >> databases do not support this. >> > It would be good to get the ClassCastException specified when you get time. Yes will do once the ccc is approved on my list > > I looked at the latest webrev and it seems okay to me. > SQLInputImpl.getNextAttribute would be bit better if it only incremented idx > after retrieving an attribute but that is existing code. Minor intent issue > in SQLOutputImpl. writeNClob. > fixed the spacing. going to leave idx alone for now as but will look at this for the future Best Lance > -Alan. Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
On 15/01/2013 16:48, Lance Andersen - Oracle wrote: Here is a revision http://cr.openjdk.java.net/~lancea/8006139/webrev.01 I still have to enter the JBS entry for the javadoc clarifications (and I also found another javadoc issue that was due to incorrect cut& paste when the code was written) and ccc request As i mentioned in an earlier thread, these classes are hardly ever, if at all used and would only be used when UDTs are used and the majority of databases do not support this. It would be good to get the ClassCastException specified when you get time. I looked at the latest webrev and it seems okay to me. SQLInputImpl.getNextAttribute would be bit better if it only incremented idx after retrieving an attribute but that is existing code. Minor intent issue in SQLOutputImpl. writeNClob. -Alan.
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Thank you Ulf. I deleted the extra line on 579 Best Lance On Jan 15, 2013, at 3:45 PM, Ulf Zibis wrote: > Looks great! > > Little nit: > SQLOutputImpl.java line 579 could be dropped. > > -Ulf > > > Am 15.01.2013 17:48, schrieb Lance Andersen - Oracle: >> Here is a revision >> >> http://cr.openjdk.java.net/~lancea/8006139/webrev.01 >> >> I still have to enter the JBS entry for the javadoc clarifications (and I >> also found another javadoc issue that was due to incorrect cut & paste when >> the code was written) and ccc request >> >> As i mentioned in an earlier thread, these classes are hardly ever, if at >> all used and would only be used when UDTs are used and the majority of >> databases do not support this. >> >> >> Best >> lance >> On Jan 14, 2013, at 5:11 AM, Alan Bateman wrote: >> >>> On 13/01/2013 23:51, Lance Andersen - Oracle wrote: : >> One other thing is that the CCE has a side-effect in that it "consumes" >> the next attribute. The methods could be changed to peek at the next >> attribute but that wouldn't work without synchronization or making it >> clear in the spec that the it is not a thread-safe implementation of >> SQLInput. I really want to keep the changes to the bare minimum here as this and the other serial classes are hardly, if ever used at all. >>> I understand, but if you add a catch-all in the class description to cover >>> the CCE case then this could be part of the same paragraph. >>> >>> -Alan >> >> >> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 >> Oracle Java Engineering >> 1 Network Drive >> Burlington, MA 01803 >> lance.ander...@oracle.com >> > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Looks great! Little nit: SQLOutputImpl.java line 579 could be dropped. -Ulf Am 15.01.2013 17:48, schrieb Lance Andersen - Oracle: Here is a revision http://cr.openjdk.java.net/~lancea/8006139/webrev.01 I still have to enter the JBS entry for the javadoc clarifications (and I also found another javadoc issue that was due to incorrect cut & paste when the code was written) and ccc request As i mentioned in an earlier thread, these classes are hardly ever, if at all used and would only be used when UDTs are used and the majority of databases do not support this. Best lance On Jan 14, 2013, at 5:11 AM, Alan Bateman wrote: On 13/01/2013 23:51, Lance Andersen - Oracle wrote: : One other thing is that the CCE has a side-effect in that it "consumes" the next attribute. The methods could be changed to peek at the next attribute but that wouldn't work without synchronization or making it clear in the spec that the it is not a thread-safe implementation of SQLInput. I really want to keep the changes to the bare minimum here as this and the other serial classes are hardly, if ever used at all. I understand, but if you add a catch-all in the class description to cover the CCE case then this could be part of the same paragraph. -Alan Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Here is a revision http://cr.openjdk.java.net/~lancea/8006139/webrev.01 I still have to enter the JBS entry for the javadoc clarifications (and I also found another javadoc issue that was due to incorrect cut & paste when the code was written) and ccc request As i mentioned in an earlier thread, these classes are hardly ever, if at all used and would only be used when UDTs are used and the majority of databases do not support this. Best lance On Jan 14, 2013, at 5:11 AM, Alan Bateman wrote: > On 13/01/2013 23:51, Lance Andersen - Oracle wrote: >> : >> One other thing is that the CCE has a side-effect in that it "consumes" the next attribute. The methods could be changed to peek at the next attribute but that wouldn't work without synchronization or making it clear in the spec that the it is not a thread-safe implementation of SQLInput. >> I really want to keep the changes to the bare minimum here as this and the >> other serial classes are hardly, if ever used at all. > I understand, but if you add a catch-all in the class description to cover > the CCE case then this could be part of the same paragraph. > > -Alan Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Yes sorry if that was not clear but that is my plan with the ccc and javadoc update I will get the JBS entry and ccc submitted later today and make a few of the minor suggestions from ulf and push out the revised webrev later today or Tuesday Best Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Sent from my iPad On Jan 14, 2013, at 5:11 AM, Alan Bateman wrote: > On 13/01/2013 23:51, Lance Andersen - Oracle wrote: >> : >> One other thing is that the CCE has a side-effect in that it "consumes" the next attribute. The methods could be changed to peek at the next attribute but that wouldn't work without synchronization or making it clear in the spec that the it is not a thread-safe implementation of SQLInput. >> I really want to keep the changes to the bare minimum here as this and the >> other serial classes are hardly, if ever used at all. > I understand, but if you add a catch-all in the class description to cover > the CCE case then this could be part of the same paragraph. > > -Alan
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
On 13/01/2013 23:51, Lance Andersen - Oracle wrote: : One other thing is that the CCE has a side-effect in that it "consumes" the next attribute. The methods could be changed to peek at the next attribute but that wouldn't work without synchronization or making it clear in the spec that the it is not a thread-safe implementation of SQLInput. I really want to keep the changes to the bare minimum here as this and the other serial classes are hardly, if ever used at all. I understand, but if you add a catch-all in the class description to cover the CCE case then this could be part of the same paragraph. -Alan
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
On Jan 13, 2013, at 4:25 PM, Ulf Zibis wrote: > > Am 13.01.2013 21:08, schrieb Alan Bateman: >> Assuming CCE is long standing behavior then it would be good to update the >> spec of all the other read* methods too. Alternatively a statement in the >> class description to cover it. Yes that is my plan. > > Only using a generified version of readObject(), a CCE would be less > surprisingly to the user. The other methods then could be deprecated. Deprecation is not needed here, especially the compiler warnings. > >> >> One other thing is that the CCE has a side-effect in that it "consumes" the >> next attribute. The methods could be changed to peek at the next attribute >> but that wouldn't work without synchronization or making it clear in the >> spec that the it is not a thread-safe implementation of SQLInput. I really want to keep the changes to the bare minimum here as this and the other serial classes are hardly, if ever used at all. > > Maybe we could add a move-back method, so the stream-like behaviour would be > more clear. > Was this API ever thread-safe? No, this code has not changed since it was written by the original authors in 2003 .and it was expected the user would deal with thread safety if required. > Does it make any sense to use it multi-threaded? Not worth the time given the rarity of this class being used. There are other other areas that are more important to spend time on . I do not see a need to spend a lot of cycles on something that is barely used unless someone in the community wants take it on as activity. This can be something that can be revisited once we get past the Java SE 8 hump Best Lance > > -Ulf > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Am 13.01.2013 21:08, schrieb Alan Bateman: Assuming CCE is long standing behavior then it would be good to update the spec of all the other read* methods too. Alternatively a statement in the class description to cover it. Only using a generified version of readObject(), a CCE would be less surprisingly to the user. The other methods then could be deprecated. One other thing is that the CCE has a side-effect in that it "consumes" the next attribute. The methods could be changed to peek at the next attribute but that wouldn't work without synchronization or making it clear in the spec that the it is not a thread-safe implementation of SQLInput. Maybe we could add a move-back method, so the stream-like behaviour would be more clear. Was this API ever thread-safe? Does it make any sense to use it multi-threaded? -Ulf
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
On 13/01/2013 15:07, Lance Andersen wrote: I will enter a jbs entry on Monday for this Assuming CCE is long standing behavior then it would be good to update the spec of all the other read* methods too. Alternatively a statement in the class description to cover it. One other thing is that the CCE has a side-effect in that it "consumes" the next attribute. The methods could be changed to peek at the next attribute but that wouldn't work without synchronization or making it clear in the spec that the it is not a thread-safe implementation of SQLInput. -Alan
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Why not pass the target class, like
private T getNextAttribute(Class clazz) throws SQLException {
if (++idx >= attrib.length) {
throw new SQLException("SQLInputImpl exception: Invalid read " +
"position");
} else {
Object attr = attrib[idx];
if(clazz.isInstance(attr))
return (T)attr;
else
throw new SQLException("Incorrect atribute type:
"+clazz+" vs "+attr.getClass());
}
}
We can generate a more appropriate error (the ClassCastException is
too general, the caller may not understand what caused it)
More importantly, the type check is done much earlier. For example, in
xxx private T readNext() throws SQLException {
001 T attrib = (T)getNextAttribute();
002 lastValueWasNull = attrib == null;
xxx return attrib;
xxx }
if the wrong attribute type is requested, line 001 does not throw, so
line 002 is also executed, which is probably a bug. If line 001
throws, we won't have this bug.
Zhong Yu
On Sun, Jan 13, 2013 at 9:11 AM, Remi Forax wrote:
> On 01/13/2013 02:26 PM, Ulf Zibis wrote:
>>
>> Am 13.01.2013 13:33, schrieb Remi Forax:
>
> Additionally I'm wondering, whether getNextAttribute() could be
> generified.
Yes it probably could however I do no want to do this as part of this
change
>>>
>>>
>>> I disagree, it should not be generified, getNextAttribute() should return
>>> Object and be casted at callsite,
>>> it will more clear if a ClassCastException occurs.
>>> Relying on inference to ask the compiler to insert the cast is dubious,
>>
>>
>> I must admit, I do not understand.
>>
>> What would be wrong with that? :
>> xxx private T readNext() throws SQLException {
>> xxx T attrib = (T)getNextAttribute();
>> xxx lastValueWasNull = attrib == null;
>> xxx return attrib;
>> xxx }
>>
>> 811 public String readNString() throws SQLException {
>> 812 return readNext();
>> 813 }
>>
>> ... so why not generifying getNextAttribute() directly?
>
>
> because it's unclear in readNString() that you can have a
> ClassCastException,
> there is no cast in readNString.
>
>>
>> -Ulf
>>
>
> Rémi
>
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
On 01/13/2013 02:26 PM, Ulf Zibis wrote:
Am 13.01.2013 13:33, schrieb Remi Forax:
Additionally I'm wondering, whether getNextAttribute() could be
generified.
Yes it probably could however I do no want to do this as part of
this change
I disagree, it should not be generified, getNextAttribute() should
return Object and be casted at callsite,
it will more clear if a ClassCastException occurs.
Relying on inference to ask the compiler to insert the cast is dubious,
I must admit, I do not understand.
What would be wrong with that? :
xxx private T readNext() throws SQLException {
xxx T attrib = (T)getNextAttribute();
xxx lastValueWasNull = attrib == null;
xxx return attrib;
xxx }
811 public String readNString() throws SQLException {
812 return readNext();
813 }
... so why not generifying getNextAttribute() directly?
because it's unclear in readNString() that you can have a
ClassCastException,
there is no cast in readNString.
-Ulf
Rémi
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
I will enter a jbs entry on Monday for this Have a good weekend Best Lance -- Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Sent from my iPhone On Jan 13, 2013, at 8:37 AM, Alan Bateman wrote: > On 12/01/2013 21:56, Lance Andersen - Oracle wrote: >> Hi >> This is a review request for 8006139 which adds missing methods to >> SQLInput/Output >> >> The webrev can be found at >> http://cr.openjdk.java.net/~lancea/8006139/webrev.00/ > Lance - I think this will require a spec clarification so that it's clear > that ClassCastException can be thrown by these methods. Since this doesn't > appear to have been specifically previously then I think you might need to > check the current behavior of existing drivers (as they will need to be > changed if they don't throw CCE already). > > -Alan
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
On 12/01/2013 21:56, Lance Andersen - Oracle wrote: Hi This is a review request for 8006139 which adds missing methods to SQLInput/Output The webrev can be found at http://cr.openjdk.java.net/~lancea/8006139/webrev.00/ Lance - I think this will require a spec clarification so that it's clear that ClassCastException can be thrown by these methods. Since this doesn't appear to have been specifically previously then I think you might need to check the current behavior of existing drivers (as they will need to be changed if they don't throw CCE already). -Alan
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Am 13.01.2013 13:33, schrieb Remi Forax:
Additionally I'm wondering, whether getNextAttribute() could be generified.
Yes it probably could however I do no want to do this as part of this change
I disagree, it should not be generified, getNextAttribute() should return Object and be casted at
callsite,
it will more clear if a ClassCastException occurs.
Relying on inference to ask the compiler to insert the cast is dubious,
I must admit, I do not understand.
What would be wrong with that? :
xxx private T readNext() throws SQLException {
xxx T attrib = (T)getNextAttribute();
xxx lastValueWasNull = attrib == null;
xxx return attrib;
xxx }
811 public String readNString() throws SQLException {
812 return readNext();
813 }
... so why not generifying getNextAttribute() directly?
-Ulf
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
On 01/13/2013 01:24 PM, Lance @ Oracle wrote:
On Jan 13, 2013, at 5:56 AM, Ulf Zibis wrote:
Am 12.01.2013 22:56, schrieb Lance Andersen - Oracle:
Hi
This is a review request for 8006139 which adds missing methods to
SQLInput/Output
The webrev can be found at http://cr.openjdk.java.net/~lancea/8006139/webrev.00/
Hi,
are you paid by code lines ;-)
Additionally you have left out one opportunity
... or is it new code style, having 2 wrong indented closing braces in one line?
Yes I should have caught that and will fix
I would code:
811 public String readNString() throws SQLException {
812 String attrib = (String)getNextAttribute();
813 lastValueWasNull =attrib == null;
814return attrib;
815 }
Additionally I'm wondering, whether getNextAttribute() could be generified.
Yes it probably could however I do no want to do this as part of this change
I disagree, it should not be generified, getNextAttribute() should
return Object and be casted at callsite,
it will more clear if a ClassCastException occurs.
Relying on inference to ask the compiler to insert the cast is dubious,
-Ulf
Rémi
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Yes I just noticed this The code was written before my time I will add them though Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Sent from my iPad On Jan 13, 2013, at 7:28 AM, Ulf Zibis wrote: > > Am 12.01.2013 22:56, schrieb Lance Andersen - Oracle: >> Hi >> This is a review request for 8006139 which adds missing methods to >> SQLInput/Output >> >> The webrev can be found at >> http://cr.openjdk.java.net/~lancea/8006139/webrev.00/ > > All @since tags are missing in these classes :-( > > -Ulf >
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Am 12.01.2013 22:56, schrieb Lance Andersen - Oracle: Hi This is a review request for 8006139 which adds missing methods to SQLInput/Output The webrev can be found at http://cr.openjdk.java.net/~lancea/8006139/webrev.00/ All @since tags are missing in these classes :-( -Ulf
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Sent from my iPad
On Jan 13, 2013, at 5:56 AM, Ulf Zibis wrote:
> Am 12.01.2013 22:56, schrieb Lance Andersen - Oracle:
>> Hi
>> This is a review request for 8006139 which adds missing methods to
>> SQLInput/Output
>>
>> The webrev can be found at
>> http://cr.openjdk.java.net/~lancea/8006139/webrev.00/
>
> Hi,
>
> are you paid by code lines ;-)
> Additionally you have left out one opportunity
> ... or is it new code style, having 2 wrong indented closing braces in one
> line?
>
Yes I should have caught that and will fix
> I would code:
>
> 811 public String readNString() throws SQLException {
> 812 String attrib = (String)getNextAttribute();
> 813 lastValueWasNull =attrib == null;
> 814return attrib;
> 815 }
>
> Additionally I'm wondering, whether getNextAttribute() could be generified.
Yes it probably could however I do no want to do this as part of this change
>
> -Ulf
>
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Hi again,
as you imported java.net.URL, you do not need to outline it in the code any
more, e.g. line 769.
Am 13.01.2013 12:13, schrieb Ulf Zibis:
... and again more general:
xxx private T readNext() throws SQLException {
xxx T attrib = (T)getNextAttribute();
xxx lastValueWasNull = attrib == null;
xxx return attrib;
xxx }
811 public String readNString() throws SQLException {
812 return readNext();
813 }
Again more simplified:
141 private T getNextAttribute() throws SQLException {
142 if (++idx >= attrib.length) {
143 throw new SQLException("SQLInputImpl exception: Invalid read "
+
144"position");
145 } else {
146 lastValueWasNull = attrib[idx] == null;
146 return (T)attrib[idx];
147 }
148 }
811 public String readNString() throws SQLException {
812 return getNextAttribute();
813 }
-Ulf
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Oops, missing spaces by Thunderbird failure, correction:
811 public String readNString() throws SQLException {
812 String attrib = (String)getNextAttribute();
813 lastValueWasNull = attrib == null;
814 return attrib;
815 }
Additionally I'm wondering, whether getNextAttribute() could be generified.
... and again more general:
xxx private T readNext() throws SQLException {
xxx T attrib = (T)getNextAttribute();
xxx lastValueWasNull = attrib == null;
xxx return attrib;
xxx }
811 public String readNString() throws SQLException {
812 return readNext();
813 }
-Ulf
Am 13.01.2013 11:56, schrieb Ulf Zibis:
Am 12.01.2013 22:56, schrieb Lance Andersen - Oracle:
Hi
This is a review request for 8006139 which adds missing methods to
SQLInput/Output
The webrev can be found at http://cr.openjdk.java.net/~lancea/8006139/webrev.00/
Hi,
are you paid by code lines ;-)
Additionally you have left out one opportunity
... or is it new code style, having 2 wrong indented closing braces in one line?
I would code:
811 public String readNString() throws SQLException {
812 String attrib = (String)getNextAttribute();
813 lastValueWasNull =attrib == null;
814return attrib;
815 }
Additionally I'm wondering, whether getNextAttribute() could be generified.
-Ulf
Re: review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Am 12.01.2013 22:56, schrieb Lance Andersen - Oracle:
Hi
This is a review request for 8006139 which adds missing methods to
SQLInput/Output
The webrev can be found at http://cr.openjdk.java.net/~lancea/8006139/webrev.00/
Hi,
are you paid by code lines ;-)
Additionally you have left out one opportunity
... or is it new code style, having 2 wrong indented closing braces in one line?
I would code:
811 public String readNString() throws SQLException {
812 String attrib = (String)getNextAttribute();
813 lastValueWasNull =attrib == null;
814return attrib;
815 }
Additionally I'm wondering, whether getNextAttribute() could be generified.
-Ulf
review request 8006139, add missing methods to javax.sql.rowset.serial.SQLInput/OutputImpl
Hi This is a review request for 8006139 which adds missing methods to SQLInput/Output The webrev can be found at http://cr.openjdk.java.net/~lancea/8006139/webrev.00/ best Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
