1
edit
Changes
no edit summary
! Status
|-
|getQuotedName * functionality is not needed - just use SQLSchemamanager.quote() the way this is done in other sql manager code
|
|}
! Status
|-
| setQueryTimeout() - *remove the condition - *it should be possible to set 0 timeout |
|-
|CancelTask -> StatementCancelationTask; *make it a top level class
|
|}
! Status
|-
|addColumn - *change the comment to "Reads a column from a JDBC driver result set and adds it to a table object". |
|-
|Move rs argument to the last position
|
|-
| The message about ignoring an index has changed - *I think the extra info in the original message that the index was ignored was important.
|
|-
|There are extensive changes to readSchema? *What is the reason for these (or is it the diff malfunctioning)?
|
|-
|getAlterColumnToken
*remove the separator flag. *Adding separators should be done in different code.Each method should do as little as possible.
|
|-
|isPortable() - *null/skipped columns should not affect portability. It is better to handle null columns in the caller.*(Methods should not try to do several tasks at once, in this case skipping invalid entries and determining portability.
This hampers method reusability.).
|
! Status
|-
|*Proxy.java -> *Wrapper.java *(this is not the proxy pattern, but more like the decorator pattern)
|
|}
|-
|MAX_FIELD_PRECISION - is this from the PostgreSQL documentation?
*The comment is incorrect. This is not 1048576*8*10 and is not 1GB, but 10MB. Remove "~~" |
|-
|MAX_LO_SIZE - Remove "~~" from the comment. This is 2GB-1.
|
|-
|BIND_STRING, BIND_BINARY - *you can optimize these with much simpler conditions than the ones in isLOB.
|
|-
|-
| The timeout management functionality requestTimeout and releaseTimeout(rename to schedule and cancel)
*should be moved to the cancelation task, as static methods. *The timer singleton instance (lazy initialized) should be there as well.
|
|-
|unwrap - <br/>*why is a pooled connection ever used there?*When could an exception occur?*Are we using the correct data source class?
|
|-
|s_PGXAConnection, s_getQueryExecutor -*what is the purpose of the conditional logic involving these?
|
|-
|appendIdentitySuffix
*Add a space after ";" for readability
|
|-
|appendLiteral
*TIMESTAMP_ORDINAL - this should be in UTC, I think that simply converting the timestamp to a string might not do it.*Try to see if using Primitive.toString() will provide the right format.
|
|-
|appendTypeConversion
*TIMESTAMP_ORDINAL - please verify that this actually formats/parses UTC timestamps.
|
|-
|indexNameMatches
*Spaces around commas are needed. Wrap the line after about 110 chars.
|
|-
|isUnicode<br/>*Use lower case SQL keywords*Use bind variables - this is what makes prepared statements cacheable by the DB*Object [] { -> no spaces here*catch(SQLException e) space before (*Do not repackage the SQLException as a runtime exception - the method already allows throwing an SQLException
|
|}
|-
|requestSavepoint
*Do we need to generate unique names for save points? (E.g. can savepoint names on different connections conflict)?*Can the JDBC driver be used to manage savepoints?*Should we use prepared statements here?*The statement cleanup should be fixed. Right now it will not close the statement if an error occurs.
|
|}
|-
|appendColumnAlteration
*"extract( epoch from" - remove space after (*Use getColumnAlterationToken*Are booleans indexable in Postgres? If not, we should use integers instead.
|
|-
|appendLOManagerTrigger
*Method JavaDoc comments are required.*What does this do?
|
|-
|appendTSIncrement
*interval' – perhaps a space before "'" for readability?
|
|-
|getGUIDExpr
*Remove "XXX: from the comment*Move the comment to the class header comment
|
|-
|isImplicitConversion
*srcType == Primitive.BOOLEAN ) - remove space before )
|
|-
|isValidColumnName
*Spaces around "-"
|
|-
|addColumn
*This might be worth refactoring further by adding a template method in order to avoid code duplication with the base class.
|
|}
