db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Fernanda Pizzorno (JIRA)" <derby-...@db.apache.org>
Subject [jira] Updated: (DERBY-690) Add scrollable, updatable, insensitive result sets
Date Fri, 17 Mar 2006 22:02:59 GMT
     [ http://issues.apache.org/jira/browse/DERBY-690?page=all ]

Fernanda Pizzorno updated DERBY-690:
------------------------------------

    Attachment: derby-690-v6.diff
                derby-690-v6.stat

Thank you for looking into the patch Øystein. Here is a new patch a answer to Øystein's
comments.

>>>1. ScrollInsensitiveResultSet:
>>>
>
>I would like Javadoc for all the attributes you have added to the
>class.

OK. Added comment for all new attributes.

>>>1.4 getNextRowFromSource()
>>>
>>>    a) Comment: I am not able to understand what is going on here
>>>       based on the comment:
>>>            - "candidate" is not mention anywhere else in this class,
>>>              and needs to be defined.  
>>>            - You mention TableScanResultSet, but I do not find any
>>>              assumption in the code about that being the only
>>>              possible type of the result set.  It is also unclear
>>>              whether you are talking about target or source here.
>>>            - Why does these ResultSets need to refer to the same row,
>>>              and why does it help to set the current row to null?  
>>>            - It is unclear to me what the relation is between the
>>>              target result set and this result set.  Is this
>>>              explained somewhere?
>>>
>>OK Re-written
>
>
>>OK The target result set that we get using
>>target = lcc.lookupCursorActivation(getCursorName()).getTargetResultSet();
>>Is the result set that is closer to the scan. Usually a TableScanResultSet
>>or IndexRowToBaseRowResultSet.
>>
>
>This confuses me.  I thought source was the result set closer to the
>scan.  At least, it is where you get the rows from.  Or are you saying
>that the target may be closer to the scan than the source?  Why is then
>called target?  If I understand you correctly, getTargetResultSet does
>not return a TargetResultSet.  This naming is very confusing.  (I know
>it is not your fault.)
>
>If I understand the comments from ActivationClassBuilder correctly,
>the target result is the result set used for position updates.  In
>that case, I do not understand why you need to change the position of
>the source result set when repositioning.  It seems to me that is only
>used to populate the hash table.

We have a hierarchy of result sets of different types. When the top 
result set needs to get a new row, it will get it from its source 
(immediately under in the hierarchy), which will get it from its 
source ... and so on, until the result set at the bottom of the 
hierarchy fetches the row from the scan.

The target result set is the one that is at the bottom of the 
hierarchy, but if a result set accesses its source result set 
which accesses its source ... and so on, it will end up indirectly
accessing the target result set.

>>>    b) Having to look up something by cursor name, indicates to me
>>>       that you increasing the coupling between modules that by design
>>>       was not intended to be coupled.  Also, this class already has a
>>>       reference to an Activation.  Is this the same or some other
>>>       Activation?
>>
>>Using activation now to get the target result set.
>
>
>I see you reverted this in the latest patch.  Why cannot the local
>activation be used?  Is it not the same activation? 
>

Yes it is. I had forgotten to check for updatable, that's why I removed 
it. Read-only result sets do not have a target.

>
>>>1.5 addRowToHashTable()
>>>
>>>    a) Javadoc:
>>>            - key is no longer positionInSource, but currentPosition.        

>>>            - typo: "this methods"
>>>            - You say that this method "is used in order to store the
>>>              new values of that row".  I think this is a bit
>>>              misleading since this method only adds row to the hash
>>>              table; it does not relate to updates.  Updates are
>>>              achieved by the caller who deletes the old version
>>>              before adding the new.
>>>
>>OK Javadoc re-written
>
>
>
>You have not adressed the last comment.
>

OK, re-written once more.

>
>>>    b) I do not like the hard-coding of constants for field numbers
>>>       and field counts.
>>
>>Changed them into constants.
>
>
>Good, but there are still some hard-coding in other methods.  

OK. I believe I got all of them now.

>I also think you can remove the comments on the assignments since
>using constants the code becomes self-explanatory. 

OK. Removed

>>>1.7 positionInLastFetchedRow()
>>>
>>>    a) No JavaDoc
>>
>>OK Added javadoc.
>
>
>Typos: Positiones, proviously, 

OK. Corrected

>>>1.8 updateCachedRow()
>>>
>>>    a) Are you sure that if a projection is needed that this will be
>>>       done by the top node of the source tree?  Could not there be
>>>       cases where a projection is needed even if source is of a
>>>       different type?  (E.g., the source of source is a
>>>       ProjectRestrictResultSet)
>>
>>The types of result sets you can get in updatable result sets seems to be
>>reduced, since there are restrictions to the type of query that can give
>>an updatable result set. I have not been able to find a case where 
>>ProjectRestrict is not the top one. The other types that we have seen in 
>>this context are TableScan and IndexRowToBaseRow which would logically
>>come under a ProjectRestrict.
>
>
>This might well be the case.  Is there some way of verifying this?
>And how easy will it be to detect the problem if new "top nodes" are
>introduced?  And would it require a major rewrite to handle that?

If we added the doBaseRowProjection as a public method in all NoPutResultSet, 
and those that are not of type ProjectRestrictRS would return the results of 
the execution of this method at their source result set, the ProjectRestrictRS 
would no longer need to be the top one. I do not think that it is a major
rewrite.

>>>    b) Javadoc: Parameter name is missing (not the only case).
>>
>>OK Found two places.
>
>
>I have not verified that this is fixed everywhere for the latest
>patch.  Could you please, generate javadoc and check warnings?

Done. Fixed all warnings concerning this patch.

>
>>>5. TableScanResultSet
>>>
>>>5.1 General:
>>>
>>>   a) I do not understand why you need to prevent re-qualification.
>>>      When will you need to read a row again?  I thought you were
>>>      getting the row from the hash table on later accesses.
>>
>>Right before an positioned update/delete is performed, CurrentOfResultSet 
>>is used to get the current row. This will re-read the row, and re-qualify 
>>before it is updated. That's the moment when we do not want to re-qualify, 
>>since changes can have been done to the row that makes it no longer 
>>qualify  and we don't want that to happen, since according to our chosen 
>>semantics for insensitive result sets, rows will remain in the result, in 
>>the same place, even if they no longer qualify according to the original
>>query.
>
>
>I still does not see how this impact the insensitivity since I would
>think the row will still be present in the hash table.  Will not the
>update be performed if the row does not qualify?
>
>How does the CurrentOfResultSet refetch the current row?  Does it
>involve the ScrollInsensitiveResultSet at all in order to get it?

There are several levels of ResultSets in between Update/DeleteResultSet
and CurrentOfResultSet, but the row to be updated/deleted comes from a
call to CurrentOfResultSet.getNextRowCore().

CurrentOfResultSet.getNextRowCore() will read a cursorRow and a currentRow.
The cursorRow comes from the CursorResultSet which in our case is a 
ScrollInsensitiveResultSet, and currentRow comes from the TargetResultSet
which can be TableScan or IndexRowToBaseRow RS.

CurrentOfResultSet uses to cursorRow to throw an exception if the cursor
is not on a row and the current row is the one returned to the result set
on top of CurrentOfResultSet.

The hierarchy will usually be:

1. Update/DeleteRS
2. DMLWriteRS
3. ProjectRestrictRS
4. CurrentOfRS

The CurrentOfRS will return the currentRow from its targetRS. ProjectRestrictRS will 
use the returned row returned by CurrentOfRS to build a new row with the information 
that is necessary for Update/DeleteRS to update/delete the row. DMLWrite does not do 
anything to the row, just returns it to Update/DeleteRS.

If the row has been updated to values that not longer qualify, for example, if I 
have a result set generated with the following statement: "SELECT a, b FROM t1 
WHERE a < 5", and I run the following code:

rs.next()
rs.updateInt(1, 10);
rs.updateRow();
rs.next();
rs.previous();
rs.updateInt(1, 15);
rs.updateRow();

when we get to the second update row, the value of a in the database and result 
set will be 10, so it will be > 5 and therefore not qualify. When UpdateResultSet 
gets the row to be updated, the targetResultSet on the CurrentOfResultSet, will 
return null as current row, because the current row has a > 10 and no longer 
qualifies. That will make the CurrentOfResultSet return null as currentRow and 
the row won't be updated, instead the application will get a "Cursor operation 
conflict" warning.

It is our design choice that once the row has been a part of the result set, 
it will continue to be a part of it even though its values are changed so that 
they no longer qualify. We chose not to have update holes in the result set, and 
therefore, we need to keep this row in the result set even though a > 10. That's 
why we want to result set to qualify only when getting a new row and not to 
qualify again the getting the current row. 

>>>   b) Both the write-up and the added comment only talks about
>>>      qualification.  It does not mention anything about
>>>      currentRowIsValid which does seem to address another issue.
>>
>>currentRowIsValid is set to the result of positioning at the rowLocation, 
>>if positioning at rowLocation returs false, the row has been deleted 
>>under our feet. Whenenver currentRowIsValid is false it means that the 
>>row has been deleted.
>>
>>Write-up updated.
>
>
>Please, add a comment to the declaration of the attribute, too.

OK. Comment added

>>>5.3 positionScanAtRowLocation()  (was: setRowLocation)
>
>
>NEW: c) I do not understand the purpose of resumeRead.  The
>description in JavaDoc (NoPutResultSet) is a bit confusing.  Is there
>some way to make this a bit more general to other types of
>NoPutResultSet?  

Removed. Moved the logic to TableScanResultSet instead.

>>>6. IndexRowToBaseRowResultSet
>>>
>>>6.1 positionFromRowLocation
>>>
>>>   b) I would like a comment that says what concept
>>>      positionFromRowLocation represents.
>>
>>Ok, comment added.
>
>
>Typo: otherwite

Ok, corrected.

>>>7. CurrentOfResultSet
>>>
>>>7.1 getNextRowCore()
>>>
>>>   b) Is skipping the call to target.getCurrentRow() necessary or
>>>      just an optimization?
>>
>>Not necessary, but if the rowLocation is NULL we know that the row
>>has been deleted.
>
>
>But is it worth the extra complexity?

Ok. Removed

>
>>>   c) I do not understand the comment for when to add the warning.
>>>      Why was there not any need for a warning earlier?
>>
>>The cursor result set for a forward-only updatable result set will
>>be sensitive to changes. The warning was not needed before because
>>if the row had been deleted under our feet, the cursor result set
>>would return NULL for cursorRow and an exception would be
>>thrown. Now that the cursor result set is a
>>ScrollInsensitiveResultSet, if a row is deleted under our feet, the
>>ScrollInsensitiveResultSet will still return the cached row from the
>>hash table (not NULL) while the target result set will return a
>>currentRow that is NULL, this is the situation that causes the
>>warning.
>
>
>I think my problem is that I have not quite understood what a target
>result set is, and especially the reasoning behind its name.
> 
>
>>>8. TemporaryRowHolderResultSet
>>>
>>>8.1 All new methods: I think the Javadoc should mention that these
>>>   methods are no-ops.
>>
>>Ok
>>
>
>Not done for positionScanAtRowLocation.

Done.

>>>13. Scan/SortBufferRowSource/SortScan
>>>
>>>13.1 Javadoc
>>>
>>>    a) Javadoc: I think you should at least put a sentence about what
>>>       the methods does in addition to referring to the methods of
>>>       ScanController.  At least for some of these classes, that
>>>       seems to be the pattern that has previously been used.
>>
>>OK. Added a line about what it is supposed to do even though not implemented
>>
>
>Not addressed in Scan.

Added javadoc to GenericScanController too.

>
>>>    b) I think the Javadoc should say that these methods will always
>>>       throw exceptions.
>>
>>OK. Added a line that the method always throws exception
>
>
>I would prefer that it also mentioned the SQLState for the exception.

Ok

>17. EmbedResultSet
>
>17.1 rowUpdated()/rowDeleted()
>
>     a) Why is checkIfClosed removed?

Probably a merge problem since checkIfClosed() was commited after the first 
patch was submitted.

>17.2 addWarning()
>
>     a) I would like javadoc for this method

Added.

>18. EmbedDatabaseMetaData
>
>18.1 othersUpdatesAreVisible()
>
>     a) The added comment was placed a bit strange. Since it supposed
>        to cover all three methods, either put it above the Javadoc
>        for the first method, or duplicate withing Javadoc for all.

Agreed. Moved above the Javadoc for the first method.


>19. EmbedConnection
>
>19.1 setResultSetConncurrency()
>
>     a) Is there any point in keeping this method around?  It just
>        returns one of it input parameters.

Removed.

>20. Added SqlState
>
>    a) Is it necessary to prefix the message with "WARNING:"?  I would
>       think that would be possible to determined from the context.
>       If kept, capitalize after colon.

Not really necessary.

>    b) According to the ODBC guide I have access to, this SqlState is
>       for "A positioned UPDATE or a positioned DELETE SQL statement
>       was specified, and no row or more than one row was updated or
>       deleted."  As far as I can tell, you will also use this warning
>       when one reposition to a deleted row.  Is that according to the
>       spec?

The warning is added in CurrentOfResultSet. This result set is only used
for positioned update/delete operations. Therefore you will not get a 
warning unless you attempt to delete/update a previously deleted row either
by a positioned update/delete or by calling ResultSet.updateRow() or
ResultSet.deleteRow() methods.

>    c) For the text of this message you have used the "subcondition"
>       specified in the SQL spec.  Is that a requirement?  I thought
>       the spec only put a requirement on the state and not the
>       associated text.  I think something more descriptive, like what
>       I found in the ODBC guide would be helpful to users.

Message changed to: "An attempt to update or delete an already deleted row 
was made: no row was updated or deleted."


> Add scrollable, updatable, insensitive result sets
> --------------------------------------------------
>
>          Key: DERBY-690
>          URL: http://issues.apache.org/jira/browse/DERBY-690
>      Project: Derby
>         Type: New Feature
>   Components: JDBC
>     Reporter: Dag H. Wanvik
>     Assignee: Dag H. Wanvik
>     Priority: Minor
>  Attachments: DERBY-690-v1.diff, DERBY-690-v1.stat, DERBY-690-v2.diff, DERBY-690-v2.stat,
SURChanges-v1.pdf, derby-690-v4.diff, derby-690-v4.stat, derby-690-v5.diff, derby-690-v5.stat,
derby-690-v6.diff, derby-690-v6.stat, sur-proposal.txt, writeup-v1.html, writeup-v2.html,
writeup-v3.html, writeup-v4.html
>
> JDBC result sets are created with three properties: type, concurrency
> and holdability. The type can be one of TYPE_FORWARD_ONLY,
> TYPE_SCROLL_INSENSITIVE and TYPE_SCROLL_SENSITIVE. The concurrency can
> be one of CONCUR_READ_ONLY and CONCUR_UPDATABLE. The holdability can
> be one of HOLD_CURSORS_OVER_COMMIT and CLOSE_CURSORS_AT_COMMIT.
> JDBC allows the full cross product of these. SQL 2003 prohibits the
> combination {TYPE_SCROLL_INSENSITIVE, CONCUR_UPDATABLE}, but this
> combination is supported by some vendors, notably Oracle.
> Currently, Derby supports JDBC result sets in a limited
> way. Holdability is supported. Furthermore, the following is
> supported: 
> 	   - forward-only, read-only 
> 	   - forward-only, updatable (update, delete, but not insert)
> 	     Also, in the network driver, support for some data types
> 	     conversions is missing.
> 	   - scroll insensitive, read-only
> We (Fernanda and Andreas will cooperate with me on this) propose a
> plan to add support for the combination:
> 	   - scroll insensitive, updatable
> for both the embedded driver and the network client driver. 
> As a part of this we would also like to add the missing insert
> operation to the {forward-only, updatable} result sets (JIRA-100), and
> remove the requirement for an explicit "FOR UPDATE" clause in the SQL
> query to achieve updatability if CONCUR_UPDATABLE is specified
> (JIRA-231).
> The full proposal text is uploaded as an attachment, including a proposed
> functional specification.
> This JIRA will  be used to track sub-issues for this effort. The sub-issues will be linked
back to this issue.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


Mime
View raw message