db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Army <qoz...@sbcglobal.net>
Subject Re: Patch again again for DERBY-167.
Date Tue, 17 May 2005 00:50:48 GMT
TomohitoNakayama wrote:
> 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.


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, " +

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(", INCREMENT BY ");

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.


View raw message