db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "TomohitoNakayama" <tomon...@basil.ocn.ne.jp>
Subject Re: Patch again again for DERBY-167.
Date Wed, 18 May 2005 11:04:10 GMT
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
         tomonaka@basil.ocn.ne.jp
         tomohito@rose.zero.ad.jp

         Naka
         http://www5.ocn.ne.jp/~tomohito/TopPage.html

*/
----- Original Message ----- 
From: "Army" <qozinx@sbcglobal.net>
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


Mime
View raw message