db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Øystein Grøvlen (JIRA) <derby-...@db.apache.org>
Subject [jira] Commented: (DERBY-690) Add scrollable, updatable, insensitive result sets
Date Wed, 15 Mar 2006 11:21:41 GMT
    [ http://issues.apache.org/jira/browse/DERBY-690?page=comments#action_12370495 ] 

Øystein Grøvlen commented on DERBY-690:
---------------------------------------

Thanks for addressing my comments.  I have only looked at code that is
related to earlier comments.  If other code has been added in the
later patches, please, point me to it. 

Basically, the changes looks good, but I still have a few questions
which is probably more about my understanding of this.

Fernanda Pizzorno (JIRA) wrote:
>> 1. ScrollInsensitiveResultSet:
>>

I would like Javadoc for all the attributes you have added to the
class.

>> 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.

> 
>>     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? 


> 
>> 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.


> 
>>     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.  

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


>> 1.7 positionInLastFetchedRow()
>>
>>     a) No JavaDoc
> 
> OK Added javadoc.

Typos: Positiones, proviously, 

>> 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?

> 
>>     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?

>> 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?

> 
>>    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.

>> 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?  

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

Typo: otherwite

>> 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?

> 
>>    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.

>> 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.

>>     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.



> 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,
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