[
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
