Mailing-List: contact derby-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: <derby-dev@db.apache.org>
Received-SPF: pass (asf.osuosl.org: local policy)
Date: Mon, 13 Feb 2006 12:02:16 +0100
From: Andreas Korneliussen <Andreas.Korneliussen@Sun.COM>
Subject: Re: [jira] Updated: (DERBY-795) After calling ResultSet.relative(0)
 the cursor loses its position
In-reply-to: <43ED252E.5030508@sun.com>
To: derby-dev@db.apache.org
Reply-to: Andreas.Korneliussen@Sun.COM
Message-id: <43F06738.1020308@sun.com>
Organization: Sun Microsystems
MIME-version: 1.0
Content-type: text/plain; charset=UTF-8; format=flowed
Content-transfer-encoding: 7BIT
User-Agent: Mozilla Thunderbird 1.0.7 (X11/20050930)
References: <457165502.1139492757890.JavaMail.jira@ajax.apache.org>
 <43EBD69E.1020806@sun.com> <43EBDC08.7040203@sun.com>
 <43EBEA4E.2080502@sun.com> <43EC76E0.1020106@sun.com>
 <43ED252E.5030508@sun.com>

David W. Van Couvering wrote:
> I agree they should be submitted as independent patches.  But they 
> should also be committable independently.
> 

They are committable independently, however it is a good idea to commit 
the patches incrementally, as they were submitted:

1. Review and commit DERBY-918. Test with existing junit tests

2. Review and commit DERBY-934. Test using junit.textui.TestRunner or by 
using the feature provided in DERBY-918.

3. Review and commit 795. Test using existing junit tests (934)


> I can't verify DERBY-918 without a test that works with the .junit test 
> type.  That's provided in DERBY-934.  Why not provide a simple sanity 
> Junit test with this patch that I can use to verify?
>

I agree that it would be great to have a simple sanity junit test.
The feature DERBY-918 is the ability to launch a junit test from the old 
harness, without having to make a new main method for every junit test.

It can be tested with the old junit tests. The fact that the old tests 
fails, (due to the fact that they were never designed to run in 
junit.textui.TestRunner), do not make them valueless in terms of 
verifying the feature.

> Then, I can't verify DERBY-795 is fixed without applying and running the 
> tests in DERBY-934.  It would be great if DERBY-795 included a test that 
> verifies it is fixed as part of the patch.
> 

DERBY-795 comes with a test in the description.  It is not part of the 
patch, since I know this will be tested as part of another patch.

DERBY-934 can be run directly from junit.textui.TestRunner's main, and 
can therefore be verified without DERBY-918.

> That would make them *truly* independent, not just in terms of what they 
> are about, but in terms of the ability of the committer to review and 
> test them independently.
>

I do not see how it affects the committers ability to review the patches 
independently.  Huge patches with a lot of soon-to-be obsolete code, is 
harder to review than small patches, which can be reviewed and committed 
incrementally.


Andreas

> David
> 
> Andreas Korneliussen wrote:
> 
>> David W. Van Couvering wrote:
>>
>>> Hi, Andreas.  Upon further thought, once we work through the comments 
>>> on DERBY-934 (still to come) I am going to go ahead and apply all 
>>> these patches at once, no need for you to do extra work.  But a 
>>> request for next time to please try and keep your patches independent 
>>> instead of interdependent.
>>>
>> That is great.
>> I think the patches are independent, however they are slightly related.
>>
>> DERBY-918: a patch for improving the test harness
>>
>> DERBY-934: a set of tests which can be run independentely using 
>> junit.textui.TestRunner or by using the test harness with patch 918
>>
>> DERBY-795: a patch for a specific bug in derby. I did not submit extra 
>> tests for this, since it is covered in 934, however there is a simple 
>> java program there, which can be run to verify the fix and the bug
>>
>> There are no compile dependencies between these patches. I think it 
>> was much better to submit these as independent patches, instead of in 
>> one huge patch.
>>
>> Andreas
>>
>>
>>> Thanks!
>>>
>>> David
>>>
>>> David W. Van Couvering wrote:
>>>
>>>> Hi, Andreas, your set of patches have a set of dependencies which 
>>>> are a little confusing at first, and ultimately somewhat intractable:
>>>>
>>>> DERBY-795 is tested by DERBY-934
>>>> DERBY-934 can't be run without, and therefore depends upon, DERBY-918
>>>>
>>>> I really can't just commit one of these patches at a time, it has to 
>>>> be all or none.
>>>>
>>>> I really would like each of these patches stand on their own, or at 
>>>> a minimum don't submit a dependent patch until the patch it depends 
>>>> upon has been committed.
>>>>
>>>> Here's what I would like to see:
>>>>
>>>> DERBY-918 comes with its own sample unit test that verifies that the 
>>>> .junit test type works.  Something very simple and easy.
>>>>
>>>> DERBY-795 has its own test that comes with it, rather than being 
>>>> tested by DERBY-934
>>>>
>>>> I have some comments on DERBY-934 too, I'll send these in a separate 
>>>> email.
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>> David W. Van Couvering wrote:
>>>>
>>>>> Crud, I missed this comment somehow, I'll look at DERBY-934 again, 
>>>>> I bet *both* my questions will be answered :)
>>>>>
>>>>> I'll get back to you if I need anything else, Andreas.
>>>>>
>>>>> David
>>>>>
>>>>> Andreas Korneliussen (JIRA) wrote:
>>>>>
>>>>>>      [ http://issues.apache.org/jira/browse/DERBY-795?page=all ]
>>>>>>
>>>>>> Andreas Korneliussen updated DERBY-795:
>>>>>> ---------------------------------------
>>>>>>
>>>>>>     Attachment: DERBY-795.diff
>>>>>>                 DERBY-795.diff
>>>>>>
>>>>>> Attached is a fix for this issue.
>>>>>> The problem is detected by the jdbcapi/SURQueryMix.junit test 
>>>>>> provided in DERBY-934, when running in embedded mode.
>>>>>>
>>>>>>
>>>>>>> After calling ResultSet.relative(0) the cursor loses its position
>>>>>>> -----------------------------------------------------------------
>>>>>>>
>>>>>>>         Key: DERBY-795
>>>>>>>         URL: http://issues.apache.org/jira/browse/DERBY-795
>>>>>>>     Project: Derby
>>>>>>>        Type: Bug
>>>>>>>  Components: JDBC
>>>>>>>    Versions: 10.1.2.1
>>>>>>> Environment: Any
>>>>>>>    Reporter: Andreas Korneliussen
>>>>>>>    Assignee: Andreas Korneliussen
>>>>>>>    Priority: Minor
>>>>>>> Attachments: DERBY-795.diff, DERBY-795.diff
>>>>>>>
>>>>>>> After calling rs.relative(0), on a scrollable ResultSet, the 
>>>>>>> cursor looses its position, and a rs.getXXX(..) fails with:
>>>>>>> SQL Exception: Invalid cursor state - no current row.
>>>>>>> Probably caused by the following logic in 
>>>>>>> ScrollInsensitiveResultSet.getRelativeRow(int row):
>>>>>>>     // Return the current row for 0
>>>>>>>         if (row == 0)
>>>>>>>         {
>>>>>>>                    if ((beforeFirst || afterLast) ||
>>>>>>>                        (!beforeFirst && !afterLast)) {
>>>>>>>                        return null;
>>>>>>>                    } else {
>>>>>>>             return getRowFromHashTable(currentPosition);
>>>>>>>                    }
>>>>>>>         }
>>>>>>> The if () will always evaluate to true, regardless of the values 
>>>>>>> of beforeFirst and afterLast
>>>>>>> Test code:
>>>>>>> import java.sql.Connection;
>>>>>>> import java.sql.DriverManager;
>>>>>>> import java.sql.ResultSet;
>>>>>>> import java.sql.SQLException;
>>>>>>> import java.sql.Statement;
>>>>>>> public class RelativeZeroIssue {
>>>>>>>           public static void main(String[] args) throws Exception {
>>>>>>>               Class.forName("org.apache.derby.jdbc.EmbeddedDriver");
>>>>>>>        Connection con = 
>>>>>>> DriverManager.getConnection("jdbc:derby:testdb2;create=true");
>>>>>>>        con.setAutoCommit(false);
>>>>>>>        try {               Statement statement = 
>>>>>>> con.createStatement();
>>>>>>>                       /** Create the table */
>>>>>>>            statement.execute("create table t1(id int)");
>>>>>>>            statement.execute("insert into t1 values 
>>>>>>> 1,2,3,4,5,6,7,8");
>>>>>>>                       Statement s = 
>>>>>>> con.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE,
>>>>>>>                    ResultSet.CONCUR_READ_ONLY);
>>>>>>>            ResultSet rs = s.executeQuery("select * from t1");
>>>>>>>            rs.next();
>>>>>>>            System.out.println(rs.getInt(1));
>>>>>>>            System.out.println(rs.relative(0));
>>>>>>>            System.out.println(rs.getInt(1));
>>>>>>>        }  finally {
>>>>>>>                       con.rollback();
>>>>>>>            con.close();
>>>>>>>        }
>>>>>>>    }
>>>>>>>  
>>>>>>> }
>>>>>>> Output from test:
>>>>>>> 1
>>>>>>> false
>>>>>>> Exception in thread "main" SQL Exception: Invalid cursor state - 
>>>>>>> no current row.
>>>>>>>        at 
>>>>>>> org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source)
>>>>>>>        at 
>>>>>>> org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source)
>>>>>>>        at 
>>>>>>> org.apache.derby.impl.jdbc.Util.generateCsSQLException(Unknown 
>>>>>>> Source)
>>>>>>>        at 
>>>>>>> org.apache.derby.impl.jdbc.EmbedConnection.newSQLException(Unknown 
>>>>>>> Source)
>>>>>>>        at 
>>>>>>> org.apache.derby.impl.jdbc.ConnectionChild.newSQLException(Unknown 
>>>>>>> Source)
>>>>>>>        at 
>>>>>>> org.apache.derby.impl.jdbc.EmbedResultSet.checkOnRow(Unknown Source)
>>>>>>>        at 
>>>>>>> org.apache.derby.impl.jdbc.EmbedResultSet.getColumn(Unknown Source)
>>>>>>>        at 
>>>>>>> org.apache.derby.impl.jdbc.EmbedResultSet.getInt(Unknown Source)
>>>>>>>        at 
>>>>>>> derbytest.RelativeZeroIssue.main(RelativeZeroIssue.java:51)
>>>>>>> Java Result: 1
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>