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 Thu, 09 Mar 2006 13:17:39 GMT
    [ http://issues.apache.org/jira/browse/DERBY-690?page=comments#action_12369638 ] 

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

I have reviewed part of this patch. I have so far reviewed the 
changes to ScrollInsensitiveResultSet and some related classes.
More will follow.  

Some of my comments may be more of a request for a general
cleanup of the implementation, and not directly related to the
changes in this patch.  For such cases, I guess we should consider
whether to file a separate jira issue.

1. ScrollInsensitiveResultSet:

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

1.2 Javadoc for classes:

    a) Should say something about that the hash table may be stored on
       disk. 
    b) I miss a description of the layout of the cached rows.

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

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?

    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?

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.

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

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

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

    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.

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?

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

1.7 positionInLastFetchedRow()

    a) No JavaDoc

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

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

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)

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

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

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.

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

    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.

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.

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

   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.

3. ProjectRestrictResultSet

3.1 setRowlocation()

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

   b) Assert instead of if-test?

3.2 doBaseRowProjection()

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

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

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


> 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, sur-proposal.txt, writeup-v1.html, writeup-v2.html, writeup-v3.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