Hello.
I send new patch for DERBY-167 , which is attached to this mail. Please review it.
Thanks for the sending the new patch.
I've reviewed it and I have the following comments. The first one is an important one, the second one is very minor.
First:
The Derby "dblook" utility will have to be modified to account for the "BY DEFAULT" keyword--right now, it generates all autoincrement columns using the "ALWAYS" keyword. See org/apache/derby/impl/tools/dblook/DB_Table.java, in the "reinsateAutoIncrement" method. Luckily, this change should be fairly easy given the changes in your patch.
Since your patch sets the "DefaultInfo" for a BY DEFAULT column to a non-null value, I think dblook can just check the "COLUMNDEFAULT" column in the SYS.SYSCOLUMNS table. If it is _not_ null, then dblook can generate "BY DEFAULT" instead of "ALWAYS".
For example, you could change the "getAutoIncStmt" statement at the top of DB_Table.java to retrieve the COLUMNDEFAULT column:
getAutoIncStmt =
conn.prepareStatement("SELECT AUTOINCREMENTSTART, " +
"AUTOINCREMENTINC, COLUMNDEFAULT FROM SYS.SYSCOLUMNS " +
"WHERE COLUMNNAME = ? AND REFERENCEID = ?");and then change the "reinstateAutoIncrement(...)" method to do the following:
.
.
.
if (autoIncCols.next()) {
long start = autoIncCols.getLong(1);
if (!autoIncCols.wasNull()) {
colDef.append(" GENERATED ");
// -- begin new logic --
String def = autoIncCols.getString(3);
colDef.append(autoIncCols.wasNull() ? "ALWAYS" : "BY DEFAULT");
// -- end new logic --
colDef.append(" AS IDENTITY (START WITH ");
colDef.append(autoIncCols.getLong(1));
colDef.append(", INCREMENT BY ");
colDef.append(autoIncCols.getLong(2));
colDef.append(")");
}
}This will make it so that dblook generates "BY DEFAULT" correctly for columns that require it.
And then it'd probably be good to add a case for this in java/testing/org/apache/derbyTesting/tests/tools/dblook_makeDB.sql, which is the script that is used for testing dblook. In that script, there's a section under the "Tables" heading that indicates "auto increment/default" test cases. You could add a simple table for the "GENERATED BY DEFAULT" case. If you have any problems getting that to work, feel free to post your questions and I'll try to help where I can.
Second (minor):
I noticed that in DefaultInfoImpl.java, the two new methods (getDefinitionBitsValue and calcIsDefaultValueAutoinc) are declared as static--is there a reason for that? I made them non-static and everything compiled, so I'm just wondering if this was intentional?
Thanks again for re-sending the patch, and please feel free to ask if you have any questions about I've written here.
Army
