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 Tue, 14 Mar 2006 13:05:42 GMT
     [ http://issues.apache.org/jira/browse/DERBY-690?page=all ]

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

    Attachment: writeup-v4.html
                derby-690-v4.diff
                derby-690-v4.stat

Here is a new version of the patch and write-up. I have chosen to call both the patch and
the write-up v4 so that the numbering will match. I have answered to Øysteins comments from
1 to 16.1.

> 1. ScrollInsensitiveResultSet:
> 
> 1.1 Imports a few classes that are not used (TargetResultSet, SQLBlob,
>   SQLVarchar).
> 

OK Removed

> 1.2 Javadoc for classes:
> 
>     a) Should say something about that the hash table may be stored on
>        disk. 

OK Added some text to javadoc 

>     b) I miss a description of the layout of the cached rows.
> 

OK Added some text to javadoc 

> 1.3 getPreviousRow()
> 
>     a) I do not understand why this code is added.
>        positionInLastFetchedRow() is called if currentRow==null, but
>        currentRow does not seem to be updated by
>        positionInLastFetchedRow().  Also, it does not seem that the
>        subsequent call to getRowFromHashTable() depend on any data
>        that is set by positionInLastFetchedRow().

Not needed: Removed.

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


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

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

>     b) I do not like the hard-coding of constants for field numbers
>        and field counts.

Changed them into constants.

> 
>     c) Why are you recalculating extraColumns every time?  Will it not
>        be the same value for the entire lifetime of the object?

OK Made extraColumns as private property in the class and set it in the 
constructor

> 
>     d) Why is there some code in comments for the assignment to
>        hashRowArray[1]? 

OK Removed!

> 
>     e) I am not quite sure, but would it not be better if position was
>        a parameter to the function?  That would make the dependency on
>        the calling code more clear.

OK, added the position as parameter to addRowToHashTable

> 
> 1.6 getRowFromHashTable()
> 
>     a) You are creating an object every time you do a lookup in the
>        hash table.  Would it be an idea to have a single SQLInteger
>        object that is reused for this purpose?

OK Created private variable positionInHashTable.

> 
>     b) Disabled code in comments.  Any reason why it is not just
>        removed?

OK Removed

> 
> 1.7 positionInLastFetchedRow()
> 
>     a) No JavaDoc

OK Added javadoc.

> 
>     b) Line exceeding 80 columns (not the only case.)

OK

> 
>     c) Assignment of currentPosition:  Could not this be done for all
>        cases at the end (outside the if statement).

OK

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

> 
>     b) Javadoc: Parameter name is missing (not the only case).

OK Found two places.

> 
> 1.9 isDeleted()/isUpdated()
> 
>     a) It does not seem to be necessary to cast the objects stored in
>        the hash table to DataValueDescriptor[] in order to access its
>        contents.  (Not doing so, could shorten some lines that are too
>        long  :-) 

OK

> 
> 1.10  setRowLocation()
> 
>     a) I am not entirely happen with the name.  It sounds more like an
>        attribute is being updated than that the underlying cursor is
>        repositioned.

OK. Renamed to positionScanAtRowLocation().

> 
>     b) Javadoc: Parameter name is missing (not the only case).

OK

> 
>     c) Would not an assert be better than if-test in this case?  It
>        seems to me that the code depends on the source being a
>        CursorResultSet.

OK Not an issue longer, moved to NoPutResultSet.

> 
> 2. UpdateResultSet/DeleteResultSet
> 
>    a) Why is updateCachedRow()/markCachedRowAsDeleted() made general
>       methods on NoPutResultSet?  That requires touching a lot of
>       classes not concerned with scrollable cursors.  Would it not be
>       possible to detect that the source is a
>       ScrollInsensitiveResultSet, and do the necessary casting in this
>       case.  If you insist on making these general methods, I would
>       suggest calling them updateRow/markRowAsDeleted and let each
>       ResultSet do whatever it needs to do when rows are
>       updated/deleted.

ScrollInsensitiveResultSet is never the source, but the source of the
source ... of the source. I have renamed the two methods as you suggested.

> 
>    b) Is there a reason why you place the calls in open() and not in
>       collectAffectedRows()?

I have moved the calls to collectAffectedRows() and update/deleteDefferedRows().
I had called them in open so that I would only need to call them once for each
update, but if later other types of result sets need to use these methods it
is better that they are called after each updated row instead of only once. For
positioned updated, it would only be one affected row, so it would not be a
problem to call the methods from open().

> 
>    c) You make the assumption that only a single row is affected.
>       This may be the case for scrollable cursors, but generally
>       open() may update/delete many rows.  I think that needs to be
>       explained in the code.

For SUR we are only interested in positioned updates which would result
in one row being updated, but I have moved the method calls so that they
can be used for other types of result sets later if necessary.

> 
> 3. ProjectRestrictResultSet
> 
> 3.1 setRowlocation()
> 
>    a) Javadoc: Parameter name is missing (not the only case).

Changed to see NoPutResultSet.setRowLocation and added specific
comments to those implementations that needed it.

>    b) Assert instead of if-test?

Not needed anymore since it is a part of NoPutResultSet.

> 3.2 doBaseRowProjection()
> 
>    a) Javadoc: Parameter name is missing (not the only case).

OK

>    b) The test for (source!=null) is not necessary.

Agree. Changed.

> 
> 4. NoPutResultSet/CursorResultSet
> 
>    a) I have not really understood the relationship between
>       CursorResultSet and NoPutResultSet.  Will there be classes that
>       implement CursorResultSet but not NoPutResultSet, and vice
>       versa?

There are result sets that are NoPut but not Cursor, the opposite
does not happen.


>    b) Originally CursorResultSet has only get...() methods,
>       getRowLocation() and getCurrentRow().  You have added
>       setRowLocation(), but setCurrentRow() is part of NoPutResultSet.
>       This is a bit assymetric.  If you had added setRowLocation to
>       NoPutResultSet you would also have avoided a lot casting by
>       callers.

Moved to NoPutResultSet.

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

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

>5.2 getRowLoaction()/getCurrentRow()
>
>    a) It seems like you have changed the behavior of these functions.
>       Should not the JavaDoc be updated to reflect that?  (e.g., when
>       null is returned seems to have changed) 

OK The javadoc was not updated from before for getCurrentRow(). Updated 
both javadocs.

>5.3 setRowLocation()
>
>    a) It seems like the comment relates to the testing on isKeyed.
>       This relation could have been more obvious.  

OK. Changed the comment.

>    b) Thinking about B-Tree scans: What if a query does not need to
>       access the base table only; the index.  (That is, all selected
>       columns are part of the index.)  How does the re-positioning
>       work in this case?

The plan is to remove that optimization if the result set is of type
scrollable insensitive updatable. This seems to also be a problem for
forward-only result set, jira issue DERBY-1087 describes the problem.

>6. IndexRowToBaseRowResultSet
>
>6.1 positionFromRowLocation
>
>    a) Same question as for TableScanResultSet, when do you need to
>       refetch the row from the heap?

Same answer as for 5.1

>    b) I would like a comment that says what concept
>       positionFromRowLocation represents.

Ok, comment added.

>6.2 getCurrentRow()
>
>    a) The diff would have meen much smaller if you had structured
>       this as follows:
>
>       if (positionFromRowLocation) {
>           <new code>
>           return currentRow;
>       }
>       <old code>

OK

>    b) A comment on why your are testing on positionFromRowLocation
>       would be nice.

OK

>7. CurrentOfResultSet
>
>7.1 getNextRowCore()
>
>    a) I can not find that these changes is listed in the list of code
>       changes in the writeup.

write-up updated.

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

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

>    d) Typo: coursor

OK

>7.2 updateCachedRow()/markCachedRowAsDeleted()
>
>    a) Isn't the cursor already available as an object attribute?

Yes, OK.

>    b) Another instance of missing parameter name in Javadoc

OK

>8. TemporaryRowHolderResultSet
>
>8.1 All new methods: I think the Javadoc should mention that these
>    methods are no-ops.

Ok

>9. NoPutResultSetImpl
>
>9.1 Imports:  Why do you need to import CursorResultSet?

OK. Removed

>9.2 setCurrentRow()
>
>    a) Why have you removed 'final'?  I cannot find that you have
>       added any method that overrides this method.

OK. Put final back.

>    b) I guess it makes sense to set its currentRow here, but why was
>       it not necessary before?

The change was done so that from now on we do not need to do this:
	currentRow = XXX;
	setCurrentRow(XXX);
you can just do:
	setCurrentRow(XXX);

>9.3 updateCachedRow()/markCachedRowAsDeleted()
>
>    a) I think the Javadoc should mention that these methods are
>       no-ops.
>

OK

>9.4 setRowLocation()
>
>    a) Another instance of missing parameter name in Javadoc

OK

>    b) Typo: Positiones

OK

>    c) I do not understand the call to targetResultSet.
>       targetResultSet is as far as I can tell always an
>       InsertResultSet which, according to the javadoc, is used to
>       insert rows from a source into a base table.  How is this
>       relevant to what you are doing?

Never reached, removed.

>10. NormalizeResultSet
>
>10.1 updateCachedRow()
>        
>     a) Another instance of missing parameter name in Javadoc
>

OK


>11. ScanController
>
>11.1 fetchWithoutQualify()
>
>     a) I think the Javadoc should be more verbose and at least list
>        parameters and contain "@see fetch".  I also think it would
>        have been nice to be a bit more verbose on what not applying
>        qualifiers mean.

OK. Added more comments.

>     b) I would have placed this new method below fetch

OK. Moved

>11.2 positionAtRowLocation()
>
>     a) Why can not clients use the existing reopenScanByRowLocation
>        instead?

Part of 1067 now, Andreas will comment on it.

>12. GenericScanController
>
>12.1 reopenScanByRecordHandleAndSetLocks()
>
>     a) If I have understood things correct, when a scan is initially
>        opened, the first row is not locked.  Locking happen on the
>        subsequent next().  Why could not a similar scheme be used
>        here? That is, reopen positions just before the specified row
>        and a subsequent call to next is performed to actually lock
>        it.  Looking at fetchRows() and the methods it calls, there
>        seems to already exist code to handle a repositioned scan.
>        (The combination of SCAN_INIT and a set record posisiton).
>
>     b) I am still a bit uncomfortable with the method names in the
>        following call path: 
>            setRowLocation() 
>                positionAtRowLocation()
>                    reopenScanByRecordHandleAndSetLocks().  
>        The lower levels makes more happen than the names of the high
>        level methods indicates.

Part of 1067 now, Andreas will comment on it.

>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

>     b) I think the Javadoc should say that these methods will always
>        throw exceptions.

OK. Added a line that the method always throws exception

>14. BTreeScan
>
>14.1 fetch()/fetchWithoutQualify()
>
>     a) I do not think it is good that both methods has the same
>        Javadoc description.

OK. Specified on WithoutQualify that it does not apply qualifiers.

>
>14.2 positionAtRowLocation ()
>
>     a) See comment 13.1
>

Part of 1067 now, Andreas will comment on it.

>15. HeapScan
>
>15.1 positionAtRowLocation()
>
>     a) See 13.1 a)

Part of 1067 now, Andreas will comment on it.

>16. DiskHashTable
>
>16.1 Constructor
>
>     a) It seems strange to me that you need special handling in order
>        to be able to store RowLocation in the hash tables that is not
>        necessary for any other types.  Is the problem the
>        getNewNull() is broken for RowLocation?  Maybe that problem
>        could be fixed in stead?

OK. Fixed getNewNUll() for HeapRowLocation.

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