1
edit
Changes
Created page with '==General== {| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;" |- ! Task ! Status |- |{ ; } undo the change to ; | |} ==Cl…'
==General==
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|{
;
} undo the change to ;
|
|}
==Classes==
===Table===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|getQuotedName functionality is not needed - just use SQLSchemamanager.quote() the way this is done in other sql manager code
|
|}
===SQLAdapter===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
| setQueryTimeout() - remove the condition - it should be possible to set 0 timeout
|
|-
|CancelTask -> StatementCancelationTask; make it a top level class
|
|}
===SQLSchemaManager===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! 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
|
|-
|I think the portable flag should not take into account columns that are not added to the table (return null in this case)?
|
|-
| 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.).
|
|-
| isWindowsCompatiable -> isWindowsCompatible
|
|}
===StatementProxy/PreparedStatementProxy===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|*Proxy.java -> *Wrapper.java (this is not the proxy pattern, but more like the decorator pattern)
|
|}
===PostgreSQLAdapter===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|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_BLOB
*Why is this code needed? Under what circumstances
*m_connection -> use getConnection instead.
*nLO -> l
You already have this value, no need to get it again later on. Use Primitive.createLong() to create the long value.
*If an error occurs, lo will not be closed. Please fix this.
* setAutoCommit - why is this modified at all? We cannot simply change the autocommit flag on a connection.
|
|-
|BIND_STRING, BIND_BINARY - you can optimize these with much simpler conditions than the ones in isLOB.
|
|-
|RequestTimeout should take into account non-positive timeouts.
|
|-
| 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 - 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
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
|
|}
===PostgreSQLPreparedStatementProxy===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|Provide a class comment
|
|-
|Prepend a comment "// inner classes"
|
|-
|Use visibility declarators on members ("protected")
|
|-
|Fold lines after about 110 chars
|
|-
|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.
|
|}
===PostgreSQLSchemaManager===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|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.
|
|}
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|{
;
} undo the change to ;
|
|}
==Classes==
===Table===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|getQuotedName functionality is not needed - just use SQLSchemamanager.quote() the way this is done in other sql manager code
|
|}
===SQLAdapter===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
| setQueryTimeout() - remove the condition - it should be possible to set 0 timeout
|
|-
|CancelTask -> StatementCancelationTask; make it a top level class
|
|}
===SQLSchemaManager===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! 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
|
|-
|I think the portable flag should not take into account columns that are not added to the table (return null in this case)?
|
|-
| 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.).
|
|-
| isWindowsCompatiable -> isWindowsCompatible
|
|}
===StatementProxy/PreparedStatementProxy===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|*Proxy.java -> *Wrapper.java (this is not the proxy pattern, but more like the decorator pattern)
|
|}
===PostgreSQLAdapter===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|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_BLOB
*Why is this code needed? Under what circumstances
*m_connection -> use getConnection instead.
*nLO -> l
You already have this value, no need to get it again later on. Use Primitive.createLong() to create the long value.
*If an error occurs, lo will not be closed. Please fix this.
* setAutoCommit - why is this modified at all? We cannot simply change the autocommit flag on a connection.
|
|-
|BIND_STRING, BIND_BINARY - you can optimize these with much simpler conditions than the ones in isLOB.
|
|-
|RequestTimeout should take into account non-positive timeouts.
|
|-
| 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 - 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
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
|
|}
===PostgreSQLPreparedStatementProxy===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|Provide a class comment
|
|-
|Prepend a comment "// inner classes"
|
|-
|Use visibility declarators on members ("protected")
|
|-
|Fold lines after about 110 chars
|
|-
|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.
|
|}
===PostgreSQLSchemaManager===
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Task
! Status
|-
|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.
|
|}