Re: Patch again again for DERBY-167.

[TomohitoNakayama]

Hello.
1st:
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.
I see.
I will modify dblook.
Thank you for your notifying :D
2nd:
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?
Yes. That was intentional.
I prefer static method because it have narrower scope than instance method.
Static method can not access instance variable.
On the contrast , calling instance method would change instance variable , 
and state of the object so on.
Then , I prefer static method than instance methood.

I know method , which are to be overridden , must be instance method.
But I think those methods which I added was not such methods.
Best regards.

/*
        Tomohito Nakayama
        [EMAIL PROTECTED]
        [EMAIL PROTECTED]
        Naka
        http://www5.ocn.ne.jp/~tomohito/TopPage.html
*/
----- Original Message ----- 
From: "Army" <[EMAIL PROTECTED]>
To: "Derby Development" <derby-dev@db.apache.org>
Sent: Tuesday, May 17, 2005 9:50 AM
Subject: Re: Patch again again for DERBY-167.


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.

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

--
No virus found in this incoming message.
Checked by AVG Anti-Virus.
Version: 7.0.308 / Virus Database: 266.11.11 - Release Date: 2005/05/16


--
No virus found in this outgoing message.
Checked by AVG Anti-Virus.
Version: 7.0.308 / Virus Database: 266.11.12 - Release Date: 2005/05/17